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

Map size_t to usize by default and check compatibility [Rebased] #2278

Merged
merged 1 commit into from
Sep 24, 2022

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Sep 22, 2022

Rebased version of #2062

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks good to me with that. Thanks!

layout.align == ctx.target_pointer_size(),
"Target platform requires --no-size_t-is-usize"
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the relevant values to the assertion error? Something like:

"Target platform requires --no-size_t-is-usize. {} size ({}) or align ({}) doesn't match target pointer size ({})", spelling, layout.size, layout.align, ctx.target_pointer_size()`

@emilio
Copy link
Contributor

emilio commented Sep 24, 2022

@pvdrz you should have the ability to merge this when ready, lmk if that doesn't work for some reason. Please squash the rustfmt commit?

@pvdrz
Copy link
Contributor Author

pvdrz commented Sep 24, 2022

@emilio I'll do the change you asked and squash everything then.

…ng#1901, rust-lang#1903)

This addresses the underlying issue identified in rust-lang#1671, that size_t
(integer that can hold any object size) isn't guaranteed to match usize,
which is defined more like uintptr_t (integer that can hold any
pointer). However, on almost all platforms, this is true, and in fact
Rust already uses usize extensively in contexts where size_t would be
more appropriate, such as slice indexing. So, it's better for ergonomics
when interfacing with C code to map the C size_t type to usize. (See
also discussion in rust-lang/rust#65473 about how usize really should be
defined as size_t, not uintptr_t.)

The previous fix for rust-lang#1671 removed the special case for size_t and
defaulted to binding it as a normal typedef.  This change effectively
reverts that and goes back to mapping size_t to usize (and ssize_t to
isize), but also ensures that if size_t is emitted, the typedef'd type
of size_t in fact is compatible with usize (defined by checking that the
size and alignment match the target pointer width). For (hypothetical)
platforms where this is not true, or for compatibility with the default
behavior of bindgen between 0.53 and this commit, onwards, you can
disable this mapping with --no-size_t-is-usize.
@pvdrz pvdrz merged commit 1b7e470 into rust-lang:master Sep 24, 2022
@pvdrz pvdrz deleted the size_t branch September 24, 2022 02:21
@bors-servo bors-servo mentioned this pull request Sep 24, 2022
dspicher added a commit to dspicher/xlsxwriter-rs that referenced this pull request Nov 30, 2022
`size_t` has been aligned with `usize` [1], rendering the
cast unnecessary.

[1] rust-lang/rust-bindgen#2278
Aeradriel pushed a commit to Aeradriel/xlsxwriter-rs that referenced this pull request May 26, 2023
`size_t` has been aligned with `usize` [1], rendering the
cast unnecessary.

[1] rust-lang/rust-bindgen#2278
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.

None yet

3 participants