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

add #[repr(transparent)] #187

Merged
merged 1 commit into from Jul 19, 2021
Merged

add #[repr(transparent)] #187

merged 1 commit into from Jul 19, 2021

Conversation

flier
Copy link
Contributor

@flier flier commented Jul 26, 2019

add #[repr(transparent)] for the the FFI layout, replace #184.

@KodrAus
Copy link
Member

KodrAus commented Jul 29, 2019

Thanks for the PR @flier!

It looks like we'll have to bump our minimum supported Rust version to 1.28.0 to support the representation. That release is about 12 months old now so I think this is a reasonable change.

@josephlr
Copy link

josephlr commented Apr 23, 2020

I think this is a reasonable change and it would help me avoid boilerplate w/ FFI stuff.

I don't think an MSRV bump is needed, we can just check the version in the build.rs like we do for const fn stuff.

@KodrAus does that sound reasonable? @flier I'm happy to take this over if you don't have time.

@flier
Copy link
Contributor Author

flier commented May 3, 2020

Hi @josephlr

I have added a bitflags_repr_transparent feature gate for the #[repr(transparent)] attribute, could you help me to review the changes?
Thanks

@elmarco
Copy link

elmarco commented Apr 22, 2021

Hi @flier, can you update the MR? thanks

@KodrAus
Copy link
Member

KodrAus commented May 16, 2021

Thanks for your patience on this one @flier. Wow I really let this sit for a long time! 1.28.0 is now really quite old so I think we should be comfortable just bumping to it if we haven't already and avoid needing to add feature gates.

I was wondering whether #[repr(transparent)] came with a bit pattern guarantee (in bitflags we currently enforce that all set bits correspond to valid flags, so not all bit patterns of the underlying integer are necessarily valid bitflags), but since non-zero types like NonZeroUsize are also #[repr(transparent)] I think we're safe here.

If you'd like to update this PR to just unconditionally add the #[repr(transparent)] attribute or if somebody else would like to add that we can merge this in.

@KodrAus
Copy link
Member

KodrAus commented May 16, 2021

In #240 we bumped all the way up to 1.45.0, so can definitely unconditionally add the #[repr(transparent)] flag now 🙂

Copy link
Member

@KodrAus KodrAus left a comment

Thanks @flier! This looks good to me.

@KodrAus KodrAus merged commit efab7f3 into bitflags:master Jul 19, 2021
@elmarco
Copy link

elmarco commented Jul 19, 2021

thanks! release time? :)

@njaard
Copy link

njaard commented Aug 11, 2021

This breaks at least the crates nix and rusqlite, because they both use #[repr(C)] on "enum structs"

@mockersf
Copy link

mockersf commented Aug 11, 2021

also wgpu-types and naga, which already added #[repr(transparent)]

error[E0692]: transparent struct cannot have other repr hints
    --> /.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-types-0.7.0/src/lib.rs:2791:12
     |
2779 | / bitflags::bitflags! {
2780 | |     /// Flags for which pipeline data should be recorded.
2781 | |     ///
2782 | |     /// The amount of values written when resolved depends
...    |
2791 | |     #[repr(transparent)]
     | |            ^^^^^^^^^^^
...    |
2812 | |     }
2813 | | }
     | |_^
     |
     = note: this error originates in the macro `bitflags::bitflags` (in Nightly builds, run with -Z macro-backtrace for more info)

@KodrAus
Copy link
Member

KodrAus commented Aug 11, 2021

Thanks for the reports everyone 👍 I've gone ahead and yanked 1.3.0 so we can fix this up. I really wish we had something like crater we could use to test things like this. But in the meantime I'll add some compilation tests that include other reprs

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

6 participants