Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Make size_t_is_usize default to true #1902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

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?

Copy link

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?

Copy link
Author

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.

Copy link
Contributor

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 is usize, binds size_t as usize 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 as usize. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

#2062 implements the above.

rustfmt_configuration_file: None,
no_partialeq_types: Default::default(),
no_copy_types: Default::default(),
Expand Down
15 changes: 14 additions & 1 deletion src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,10 @@ where
),
Arg::with_name("size_t-is-usize")
.long("size_t-is-usize")
.help("Translate size_t to usize."),
.help("Translate size_t to usize. (this is the default)"),
Arg::with_name("size_t-is-not-usize")
.long("size_t-is-not-usize")
.help("Translate size_t to platform-specific lengths."),
Arg::with_name("no-rustfmt-bindings")
.long("no-rustfmt-bindings")
.help("Do not format the generated bindings with rustfmt."),
Expand Down Expand Up @@ -815,6 +818,16 @@ where

if matches.is_present("size_t-is-usize") {
builder = builder.size_t_is_usize(true);
if matches.is_present("size_t-is-not-usize") {
return Err(Error::new(
ErrorKind::Other,
"Cannot supply both --size_t-is-usize and --size_t-is-not-usize",
));
}
}

if matches.is_present("size_t-is-not-usize") {
builder = builder.size_t_is_usize(false);
}

let no_rustfmt_bindings = matches.is_present("no-rustfmt-bindings");
Expand Down