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 option to put constified enum into module #699

Closed
tmfink opened this issue May 11, 2017 · 5 comments
Closed

Add option to put constified enum into module #699

tmfink opened this issue May 11, 2017 · 5 comments

Comments

@tmfink
Copy link
Contributor

tmfink commented May 11, 2017

Input C/C++ Header

enum my_enum {
    RED,
    GREEN,
    BLUE,
};

Bindgen Invocation

$ bindgen input.h --constified-enum '.*'

Actual Results

/* automatically generated by rust-bindgen */

pub const my_enum_RED: my_enum = 0;
pub const my_enum_GREEN: my_enum = 1;
pub const my_enum_BLUE: my_enum = 2;
pub type my_enum = ::std::os::raw::c_uint;

Expected Results

It would be nice to be able to pass an option like --constified-enum-in-mod 'my_enum' that tells bindgen to put the matching constified enum inside a module:

pub mod my_enum_mod {
    use super::my_enum_type;

    pub const RED: my_enum_type = 0;
    pub const GREEN: my_enum_type = 1;
    pub const BLUE: my_enum_type = 2;
}
pub type my_enum_type = ::std::os::raw::c_uint;

One problem that must be solved is the collision between the type and module name. Some solutions:

  1. Add a prefix/suffix to the module and/or type name. In the example, "_type" and "_mod" suffixes were added to the type module name.
    • Advantages:
      • Clearly distinguishes module and type name
    • Disadvantage:
      • Must use a different name when referring the type vs. the module name
  2. Put the type definition in the module.
    • Advantage:
      • Both the type and module share the same base name
    • Disadvantage:
      • Both the module and type cannot be imported into the same namespace (because of name collision)
      • It is awkward to refer to the type as my_enum::my_enum.

Solutions shown on play.rust-lang.org

Recommendation

Overall, I prefer appending a suffix to the type and using the original name for the module. This way, you can refer to elements of the module like my_enum::RED. I believe this will overall reduce the amount of typing required, especially since type names can often be elided.

However, this behavior can be controlled with extra options, allowing bindgen users to decide what works best for their situation. The Rust interface to bindgen could also take in callback functions to allow each enum to be handled on a case-by-case basis.

My recommendation on play.rust-lang.org

Background

I'm currently working on the Rust bindings for Capstone, a disassembly library. Because the same register (for some architectures) can have several names, different enum variants (for the register id enums) have the same discriminant value. One example is mips_reg in mips.h.

I have opted to "constify" the register enums. However, in doing so, over 1000 constants are defined in the root of the library, which makes the generated documentation unwieldy.

@fitzgen
Copy link
Member

fitzgen commented May 11, 2017

Overall this sounds reasonable to me, and would be nice to have.

We could put the type into the module without calling it the same name as the module, eg something like my_enum::Type:

pub mod my_enum {
    pub Type = ::std::os::raw::c_uint;

    pub const RED: Type = 0;
    pub const GREEN: Type = 1;
    pub const BLUE: Type = 2;
}

I have a weak preference for this form, but maybe I'm overlooking something.

@tmfink
Copy link
Contributor Author

tmfink commented May 11, 2017

@fitzgen, you make a good point, but I prefer to keep the type declaration outside of the module (at least by default).

Keeping the type outside is more consistent with bindgen's current behavior. Otherwise, bindgen users would need an extra use statement for each constified enum to avoid referring to the type as my_enum::Type.

However, this is yet another option that could be set by the user. Something like --keep-constified-enum-type-inside-module could work. We should probably give users a choice.

@fitzgen
Copy link
Member

fitzgen commented May 11, 2017

would need an extra use statement for each constified enum to avoid referring to the type as my_enum::Type.

I was imagining that is how it would be referred to. Doesn't seem worse than my_enum_type and can't result in a conflicting name if some C/C++ thing we're also generating bindings for also happens to be called my_enum_type.

However, this is yet another option that could be set by the user. Something like --keep-constified-enum-type-inside-module could work. We should probably give users a choice.

The more options we have, the more combinations there are, the more bugs will be latent and hiding. Its good to have options when we need to flip things on/off for correctness issues (eg why constified enums exists in the first place) but I'd like to gently push back on adding options to control all manner of aesthetics.

@tmfink
Copy link
Contributor Author

tmfink commented May 11, 2017 via email

tmfink added a commit to tmfink/rust-bindgen that referenced this issue Jun 11, 2017
bors-servo pushed a commit that referenced this issue Jun 21, 2017
Feature 699 constified enum module

This is a work in progress for issue #699 that adds the `--constified-enum-module` option to bindgen.

@emilio, could you give me some guidance on fixing the uses of the enum variant types? In the example below, `foo` should be replaced with `foo::Type`. I'm not sure of the proper way to rename `Item`s after the structures have been defined. My initial thought was to redefine the `CodeGenerator` trait to take a mutable reference to `item`, but that will not work because of the borrow checker. Thoughts?

Todo:
- [x] put constified enum variants in a `mod`
- [x] ensure references to constified enum `foo` are changed to `foo::Type`
- [x] handle `typedef` enums

-----

Given the input header `tests/headers/constify-module-enums.h`:
~~~c
// bindgen-flags: --constified-enum-module foo

enum foo {
  THIS,
  SHOULD_BE,
  A_CONSTANT,
};

struct bar {
  enum foo this_should_work;
};
~~~

`$ cargo run -- tests/headers/constify-module-enums.h --constified-enum-module foo --no-layout-tests` will output:
~~~rust
/* automatically generated by rust-bindgen */

pub mod foo {
    pub type Type = ::std::os::raw::c_uint;
    pub const THIS: Type = 0;
    pub const SHOULD_BE: Type = 1;
    pub const A_CONSTANT: Type = 2;
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct bar {
    pub this_should_work: foo,
}
impl Clone for bar {
    fn clone(&self) -> Self { *self }
}
~~~
@fitzgen
Copy link
Member

fitzgen commented Jul 31, 2017

This was fixed in #741.

@fitzgen fitzgen closed this as completed Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants