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

Don’t recommend empty enums for opaque types #44

Closed
wants to merge 2 commits into from

Conversation

SimonSapin
Copy link
Contributor

Fix #29

@SimonSapin
Copy link
Contributor Author

CC @skade

@Gankra
Copy link
Contributor

Gankra commented Nov 23, 2017

I literally didn't know this chapter was added??

@SimonSapin
Copy link
Contributor Author

@gankro It seems to be imported from the Book’s first edition: https://doc.rust-lang.org/book/first-edition/ffi.html

src/ffi.md Outdated
pub enum Foo {}
pub enum Bar {}
#[repr(C)] pub struct Foo { private: [u8; 0] }
#[repr(C)] pub struct Bar { private: [u8; 0] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be _priv: to avoid unused field warnings, and to match stdlib style.

I also don't really understand the point of repr(C) here? Silencing the lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, rustc warns without it. Same with [u8; 0] over ().

@Gankra
Copy link
Contributor

Gankra commented Nov 23, 2017

(heading to work) we should explain what each of the 3 weird things in this type accomplish, if we don't already.

@RalfJung
Copy link
Member

Given that empty types have been recommended here previously, and may still be recommended in various blogs posts or so out there, maybe there should be an explicit section explaining that empty enums should NOT be used?

@Gankra
Copy link
Contributor

Gankra commented May 3, 2018

ping @SimonSapin what do we want to do about this

@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2018

I opened #82 to address the remaining concerns.

@SimonSapin
Copy link
Contributor Author

Oops, sorry I let this slip through the cracks. Thanks Ralf for taking over. #82 looks good so let’s close for that one.

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.

3 participants