Feature 699 constified enum module by tmfink · Pull Request #741 · rust-lang/rust-bindgen

tmfink · 2017-06-10T09:57:42Z

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 Items 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:

put constified enum variants in a mod
ensure references to constified enum foo are changed to foo::Type
handle typedef enums

Given the input header tests/headers/constify-module-enums.h:

// 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:

/* 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 }
}

emilio · 2017-06-10T11:12:07Z

@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 Items 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?

The name used for the type is generated by namespace_aware_canonical_path, which can run arbitrary logic and see the options, so tweaking those functions in src/ir/item.rs is the way to go.

emilio

Looks generally ok, but needs some more tests.

In particular:

Test that it works with C++ namespaces.
Test that it works with typedef enum foo bar; bar ty
Test that it works in other contexts like pointers, function arguments, function return values...

emilio · 2017-06-12T08:17:02Z

src/ir/item.rs

                                      -> Vec<String> {
        let path = self.canonical_path(ctx);
+        if let super::item_kind::ItemKind::Type(ref type_) = self.kind {
+            if let &TypeKind::Enum(ref enum_) = type_.kind() {


nit: if let TypeKind::Enum(ref enum_) = *type.kind() looks more consistent with the rest of the code.

emilio · 2017-06-12T08:20:26Z

src/ir/item.rs

+            if let &TypeKind::Enum(ref enum_) = type_.kind() {
+                if enum_.is_constified_enum_module(ctx, self) {
+                    // Type alias is inside a module
+                    return vec![path.last().unwrap().clone(),


This probably won't work for C++ namespaces, you need to use let mut path above and path.push(CONSTIFIED_ENUM_MODULE_REPR_NAME.into()) here, then keep executing the rest of the logic of this function.

Though it feels somewhat hacky to put it here...

emilio · 2017-06-16T19:44:17Z

src/ir/item.rs

                                      -> Vec<String> {
-        let path = self.canonical_path(ctx);
+        let mut path = self.canonical_path(ctx);
+        let mut is_constified_module_enum = false;


Let's add a helper function that returns this instead.

emilio · 2017-06-16T19:46:02Z

src/ir/item.rs

+        }
        if ctx.options().enable_cxx_namespaces {
+            if is_constified_module_enum {
+                path.push(CONSTIFIED_ENUM_MODULE_REPR_NAME.into());


I think this should be in canonical_path instead, and we'd just keep this as it was. This will make this function way less error-prone, see below.

I think the check should be in namespace_aware_canonical_path. If canonical_path appends ::Type to the path, then namespace_aware_canonical_path will have to also check if the item is a constified module enum. Both functions would need to be changed instead of just namespace_aware_canonical_path.

Well, I guess, but that relies on canonical_path users to not care about this... Which I guess holds, for now. Fine then.

emilio · 2017-06-16T19:51:12Z

tests/headers/constify-module-enums-types.hpp

+typedef foo_alias1 foo_alias2;
+
+typedef struct bar {
+  foo member1;


Add the new option to ns1::foo2, and add an usage of it here, I'm pretty sure it'd be broken.

I added the test; the behavior seems to be correct

Not with namespaces disabled. The following test still fails:

// bindgen-flags: --constified-enum-module one_Foo namespace one { enum class Foo { Variant1, Variant2, }; } class Bar { one::Foo* baz; };

emilio · 2017-06-16T19:52:19Z

src/ir/item.rs

+            return vec![path.last().unwrap().clone(),
+                        CONSTIFIED_ENUM_MODULE_REPR_NAME.into()];
+        }
        if ctx.options().disable_name_namespacing {


I don't think this branch is correct.

emilio · 2017-06-16T19:54:12Z

Looks pretty good, btw!

emilio · 2017-06-17T19:32:48Z

src/ir/item.rs

+            }
            return path;
        }
+        if is_constified_module_enum {


To fix the last test-case I commented about, you should remove this early return and let it arrive to the last return.

For that to work, you need to add the REPR_NAME to the path, so this method could be structured like:

if is_constified_module_enum { path.push(CONSTIFIED_ENUM_MODULE_REPR_NAME.into()); } if ctx.options().enable_cxx_namespaces { return path; } if ctx.options().disable_name_namespacing { let trailing_segments = if is_constified_module_enum { 2 } else { 1 }; return path[..path.len() - trailing_segments].iter().cloned().collect(); } return vec![path[1..].join("_")];

emilio

Oh, another thing we should test is when the name of the variant is the same as the current REPR_NAME. I guess we should mangle the variant in that case.

Test-case:

// bindgen-flags: --constified-enum-module MyEnum

enum MyEnum {
  Type,
  Type_,
  Bar,
};

Though actually, that just works on rust1, apparently, so let's just add the test-case.

Item::is_constified_enum_module() only returns true for the base type, not for "layers" of aliases. Added a "simple alias" test and added content to the types test.

tmfink · 2017-06-19T03:52:55Z

@emilio I hit the points you mentioned; I ended up handling Item::namespace_aware_canonical_path a little differently. I also had to complicate Item::is_constified_enum_module to handle recursive aliases.

emilio · 2017-06-19T15:42:06Z

src/ir/item.rs

    }
+
+    /// Returns whether the item is a constified module enum
+    fn is_constified_enum_module(&self, ctx: &BindgenContext) -> bool {


So what you're trying to do here is resolving through ResolvedTypeRefs, but not through Alias, right?

If so, instead of reinventing the wheel, you can do something like:

self.id.into_resolver() .through_type_refs() .resolve(ctx)

And keep the comment on why not through type alias.

We should arguably unify that and the canonical_type business, but that also handles some template stuff that may be nontrivial, so it's worth doing on a followup.

The check needs to "peel back" one layer of Aliases. The issue with the resolve() function is that it resolves through all of the Aliases.

I think that implementation should remain as is.

Hmm... I see. I guess I'm not sure why would making an alias claim it's a constified enum module be necessary.

I guess I'll poke a bit at the code this evening when I have the chance and see which tests break with that.

Hmm... Ok, so what breaks in that case is the typedef enum case... fun. I still think we should be able to do better, though...

What about modifying ItemResolver to optionally disable recursive resolving? More generally, ItemResolver could even take a positive integer indicating through how many layers to resolve.

This way, is_constified_enum_module() could be simplified to use ItemResolver.

I don't think this is about the number of layers to resolve. I think the logic in is_constified_enum_module is wrong, and the "alias to resolved type ref to alias" logic is just masking it.

Here's what I came up with, I think this is what you're really trying to do with that function:

/// Returns whether the item is a constified module enum. fn is_constified_enum_module(&self, ctx: &BindgenContext) -> bool { // Do not jump through aliases, except for aliases that point to a type // with the same name, since we don't generate code for them. let item = self.id.into_resolver().through_type_refs().resolve(ctx); let type_ = match *item.kind() { ItemKind::Type(ref type_) => type_, _ => return false, }; match *type_.kind() { TypeKind::Enum(ref enum_) => { enum_.is_constified_enum_module(ctx, self) } TypeKind::Alias(inner) => { // TODO(emilio): Make this "hop through type aliases that aren't // really generated" an option in `ItemResolver`? let inner_item = ctx.resolve_item(inner); let name = item.canonical_name(ctx); if inner_item.canonical_name(ctx) != name { return false; } return inner_item.is_constified_enum_module(ctx) } _ => false, } }

Feel free to address the TODO if you want, btw :)

emilio

r=me, with that nasty bit of code simplified :)

@emilio

Used suggested code from @emilio and also added a test for an alias to an anonymous enum.

emilio

Looks great, thanks!

And sorry it took so long to get landed :)

emilio · 2017-06-21T12:03:06Z

@bors-servo r+

bors-servo · 2017-06-21T12:03:06Z

📌 Commit 814d28e has been approved by emilio

bors-servo · 2017-06-21T12:03:09Z

⌛ Testing commit 814d28e with merge 480c2d1...

@emilio

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 } } ~~~

bors-servo · 2017-06-21T12:33:17Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing 480c2d1 to master...

highfive added the S-awaiting-review label Jun 10, 2017

Adds --constified-enum-module option per rust-lang#699

5762339

tmfink force-pushed the feature-699-constified-enum-module branch from ab6b6f4 to 5762339 Compare June 11, 2017 20:34

Convert comment to a doc comment

0826846

emilio reviewed Jun 12, 2017

View reviewed changes

Add typedef/namespace tests for module const enum

fb9959a

emilio added S-needs-code-changes and removed S-awaiting-review labels Jun 15, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 16, 2017

tmfink added 2 commits June 16, 2017 01:32

Add more constified module enum tests

977ec6f

Add constified module enum template test

de155b9

emilio reviewed Jun 16, 2017

View reviewed changes

Refactor namespace code and test

5f4b730

emilio reviewed Jun 17, 2017

View reviewed changes

emilio added S-needs-code-changes and removed S-awaiting-review labels Jun 17, 2017

tmfink added 2 commits June 18, 2017 19:56

Fix recursive aliases to const module enum

4b10529

Item::is_constified_enum_module() only returns true for the base type, not for "layers" of aliases. Added a "simple alias" test and added content to the types test.

Test const mod enum variants shadowing "Type"

465e242

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 19, 2017

emilio reviewed Jun 19, 2017

View reviewed changes

emilio approved these changes Jun 19, 2017

View reviewed changes

Simplify is_constified_enum_module

814d28e

Used suggested code from @emilio and also added a test for an alias to an anonymous enum.

emilio approved these changes Jun 21, 2017

View reviewed changes

highfive assigned emilio Jun 21, 2017

highfive removed the S-awaiting-review label Jun 21, 2017

highfive added the S-awaiting-merge label Jun 21, 2017

bors-servo merged commit 814d28e into rust-lang:master Jun 21, 2017

highfive removed the S-awaiting-merge label Jun 21, 2017

fitzgen mentioned this pull request Jul 31, 2017

Add option to put constified enum into module #699

Closed

tmfink mentioned this pull request Aug 21, 2023

[WIP] Compile-time Target Architecture Tags capstone-rust/capstone-rs#143

Draft

5 tasks

Conversation

tmfink commented Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Jun 10, 2017

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Jun 16, 2017

Uh oh!

emilio Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

tmfink commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmfink commented Jun 10, 2017 •

edited

Loading

emilio Jun 17, 2017 •

edited

Loading