Skip to content

Represent enums as constants instead of enums #480

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

Closed
sdroege opened this issue Oct 28, 2017 · 5 comments
Closed

Represent enums as constants instead of enums #480

sdroege opened this issue Oct 28, 2017 · 5 comments

Comments

@sdroege
Copy link
Member

sdroege commented Oct 28, 2017

From the bindgen 0.31.0 release notes

C/C++ enums are now translated into constants by default, rather than Rust enums. The old behavior was a big footgun because rustc assumes that the only values of an enum are its variants, whereas a lot of C/C++ code uses random values as enums. Put these two things and it leads to undefined behavior. Translating C/C++ enums into Rust enums is still available with the --rustified-enum CLI flag and bindgen::Builder::rustified_enum("") builder method. rust-lang/rust-bindgen#758

@EPashkin
Copy link
Member

@sdroege You don't understand, how it solves problem with function accepts enum values? Now it accept any int?

@sdroege
Copy link
Member Author

sdroege commented Oct 28, 2017

Basically what has to be done to make this safe is that the sys bindings use integers and constants. And the non-sys bindings use enums with the Unknown variant we have already, and convert from/to integers with the conversion traits.

The important part is that we should never have an out of range value anywhere, currently we can at the sys layer.

We should try to fix that before the next release. We already have the constants in the sys bindings, just the enums would go away.

@EPashkin
Copy link
Member

If it only for sys, then IMHO this good, while slightly loss in declarability it removes UB

@sdroege
Copy link
Member Author

sdroege commented Oct 28, 2017

OK, I'll prepare a PR :)

sdroege added a commit to sdroege/gir that referenced this issue Nov 4, 2017
The C code might be extended by additional values without the bindings
being updated, which then is considered undefined behaviour on the Rust
side. As such we must use plain integers here.

The enums stay as enums in the non-sys crates and the conversion traits
ensure that the conversion from the integers to the actual enum values
is done correctly, while storing unknown values in a hidden enum
variant.

Fixes gtk-rs#480
@EPashkin
Copy link
Member

EPashkin commented Nov 4, 2017

As pangocairo don't contains enums, seems gtk last PR.

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

No branches or pull requests

2 participants