-
Notifications
You must be signed in to change notification settings - Fork 318
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
Support rename rule for union body members #751
Conversation
@emilio What's the time table for getting something like this into a release? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a test, and I think the option naming could be more consistent? But let me know if you feel strongly, I guess just rename_fields
could be seen as renaming the foo
in Bar { foo: i32, bar: i32 }
, even though it wouldn't.
In terms of getting it to a release, once it's merged I'm happy to publish it right away.
src/bindgen/config.rs
Outdated
@@ -559,6 +559,9 @@ impl StructConfig { | |||
pub struct EnumConfig { | |||
/// The rename rule to apply to the name of enum variants | |||
pub rename_variants: RenameRule, | |||
/// The rename rule to apply to the names of union body members. | |||
/// Applied before rename_variants rename rule. Defaults to SnakeCase. | |||
pub rename_union_body_members: RenameRule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is not for Rust unions (as in union Foo {}
but C/C++ unions in an enum). I think this should just be rename_fields
for consistency with config.{struct,union}.rename_fields
, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename_variant_name_field
would be more explicit? I dunno.
If you can elaborate a bit on your use case it'd be great, too (maybe you can post an example of what you want to do exactly and how the renaming interferes?) |
Whenever I run 'cargo test' I get errors like " panicked at 'build should succeed: CargoExpand("expand-dep", Compile("" which seem unrelated to my changes. I looked at contributing.md and only saw something about installing cython, which I did. |
On which OS? Does running |
Thanks, that ended up being the issue! I think this should work now. Let me know if I need other tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
Ah, rustfmt is complaining about the trailing whitespace, but I can address manually, assuming all other tests pass. |
v0.22.0 * Support rename rule for union body members (mozilla#751). * constant: Add support for associated constant expressions (mozilla#752). * Fix regression in CamelCase rename rule (should be lowerCamelCase) (mozilla#750). * enumeration: simplify standard types in variants (mozilla#749). * Avoid generating and writing bindings when called recursively (mozilla#747). * Cython: Omit per-variant tags in unions generated for Rust enums (mozilla#748). * Update various dependencies. # Conflicts: # tests/rust/enum.toml
I am trying to use C++ macros to generate accessors for the fields in a rust enum. Since they are converted to SnakeCase by default, its impossible to write a macro that does this using only the original enum variant name.
Can we add an option to support different rename rules for the union body members as well?