Skip to content

Make size_t_is_usize default to true#1902

Closed
dkg wants to merge 1 commit intorust-lang:masterfrom
dkg:default_to_size_t_is_usize
Closed

Make size_t_is_usize default to true#1902
dkg wants to merge 1 commit intorust-lang:masterfrom
dkg:default_to_size_t_is_usize

Conversation

@dkg
Copy link
Copy Markdown

@dkg dkg commented Oct 16, 2020

This fixes: #1901

@dkg dkg force-pushed the default_to_size_t_is_usize branch from b7d0f5f to 82086e2 Compare October 16, 2020 17:13
Copy link
Copy Markdown
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.

This also needs a test of course, but not opposed to this in principle.

record_matches: true,
rustfmt_bindings: true,
size_t_is_usize: false,
size_t_is_usize: true,
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@bors-servo
Copy link
Copy Markdown

☔ The latest upstream changes (presumably ca78ae9) made this pull request unmergeable. Please resolve the merge conflicts.

@ojeda
Copy link
Copy Markdown
Contributor

ojeda commented Sep 24, 2022

@emilio @pvdrz Done through #2278.

@emilio emilio closed this Sep 24, 2022
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.

size_t_is_usize should default to true

6 participants