Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate smaller enum types #180

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Conversation

ian-h-chamberlain
Copy link
Member

Closes #166 I guess?

Related to #178 (review) and some of the changes in #162

By passing -fshort-enums to clang, we can get the same smaller types that gcc generates for some of these enums.

This also makes sure the layout tests have the same smaller sizes, so the expected sizes now match the actual sizes of the same C structs.


One thing this doesn't do is truly check that GCC is generating the same everything... ideally, I think we'd want to link against a C lib that had a bunch of APIs like this or something:

size_t sizeof_errorType() { return sizeof(errorType); }
size_t alignof_errorType() { return alignof(errorType); }
size_t offsetof_errorType() { return offsetof(errorConf, type); }

and then a Rust test like

extern "C" {
	fn sizeof_errorType() -> libc::size_t;
	fn aligneof_errorType() -> libc::size_t;
	fn offsetof_errorType() -> libc::size_t;
}

#[test]
fn layouts() {
	assert_eq!(mem::size_of<errorType>(), sizeof_errorType());
	assert_eq!(mem::align_of<errorType>(), alignof_errorType());
	assert_eq!(mem::offset_of!(errorConf, type_), offsetof_errorType());
}

But without bindgen doing something like this for us, this seems like a tricky thing to write unless we just do it manually. So for now I think I'll kinda ignore that and leave it as a future enhancement if we really need it.

By passing -fshort-enums to clang, we can get the same smaller types
that gcc generates for some of these enums.

This also makes sure the layout tests have the same smaller sizes, so
the expected sizes now match the actual sizes of the same C structs.
Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, quite a lot of size changes. I must say it scares me quite a bit to have all of these representations changed on a whim, but it makes sense seeing how there is some inconsistency between the Rust and C repr.

Praying that this won't break some hidden API 🙏 (at least we spare some space compared to before 🤷‍♂️ ).

@FenrirWolf
Copy link
Member

If anything the scary/weird part is that gcc seems to use short enums by default for arm-none-eabi while clang doesn't. You'd think they would agree on those settings, but at least we can do it manually this way. We'll probably want a similar PR for citro3d-rs too

@ian-h-chamberlain ian-h-chamberlain merged commit e3250bb into master Apr 23, 2024
4 checks passed
@ian-h-chamberlain ian-h-chamberlain deleted the fix/bindings-short-enums branch April 23, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate constants to match the types they're used as
3 participants