-
Notifications
You must be signed in to change notification settings - Fork 744
Make size_t_is_usize default to true #1902
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
Conversation
844d99d
to
b7d0f5f
Compare
Fixes: rust-lang#1901 (see also: rust-lang#1903)
b7d0f5f
to
82086e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs a test of course, but not opposed to this in principle.
@@ -1869,7 +1869,7 @@ impl Default for BindgenOptions { | |||
time_phases: false, | |||
record_matches: true, | |||
rustfmt_bindings: true, | |||
size_t_is_usize: false, | |||
size_t_is_usize: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this just works as-is. How would a non-cli user signal that they really care about whether size_t
is usize
?
Also, I think if we change the default back we should add some sanity-check / static_assert, effectively, that they're the same.
I think this should be relatively straight-forward. size_t_is_usize
needs to be a tri-state (Equal
, Different
, Default
or something like that) where the default behavior is just assume they are equal but add something like this to the bindings when we generate layout assertions:
#[test]
fn __bindgen_assumes_size_t_is_usize() {
assert_eq!(size_of::<usize>(), size_of::<size_t>(), "...");
assert_eq!(align_of::<usize>(), align_of::<size_t>(), "...");
}
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but add something like this to the bindings when we generate layout assertions:
Should it run in generate()
so that the build scripts can fail on the weird platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not deep enough in the rust internals to know the right way to resolve these questions, but yeah, i agree that building should fail on weird platforms where this is necessary. I think #1903 describes the need for that failure. But the longer that bindgen ships with the "false" default, the worse impact it'll have in the downstream ecosystem. Please don't wait on answers for me in how to change to a more sensible default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be merged along with / after #1903.
I'm not sure I understand the tristate - why would you need to insist that they're different?
The behavior that I think makes the most sense is:
- By default, bindgen verifies (not assumes) that
size_t
isusize
, bindssize_t
asusize
if so, and fails the build if not. This is basically a new behavior. (It'd be fine to cause the generated bindings to fail to compile with a static assertion, but if possible, it's nicer to do it at generation time, yes.) - If you pass some option, bindgen binds
size_t
as the concrete integer size on your platform (u64
or whatever), and doesn't bother checking whether it's the same asusize
. This is the current default behavior today.
That's two options, and I think that covers everything.
And I think the existing option --size_t-is-usize
from #1720 should be ignored for backwards compatibility.
I agree about impact on the downstream ecosystem - it's been over a year since the original change, and this is a breaking change in the generated bindings, so the longer it stays the more bindings change (and are impacted by this also being a breaking change). Happy to open a PR with the behavior above if everyone thinks it sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2062 implements the above.
☔ The latest upstream changes (presumably ca78ae9) made this pull request unmergeable. Please resolve the merge conflicts. |
This fixes: #1901