Only whitelist items for which we intend to generate bindings #827

fitzgen · 2017-07-19T22:58:18Z

This means that we only need to check in one place, rather than at ever call site.

This makes it so that we don't generate methods for the test case, but if I add the --target flag like in the original #826 test case, then I get a panic about an unknown ABI. I figure this change is worth landing now, and we can land the fix for the panic as a follow up.

r? @upsuper

fitzgen · 2017-07-19T23:05:44Z

tests/expectations/tests/issue-826-generating-methods-when-asked-not-to.rs

+extern "C" {
+    #[link_name = "_ZN3Foo4testEv"]
+    pub fn Foo_test(this: *mut Foo);
+}


Oh. Woops. Need to think about a proper fix some more.

Oh, I was already reviewing... I suggested a fix below, let me know if it sounds reasonable to you.

emilio

I can see the reasoning for this change, and this looks good.

However, it doesn't fix the reported bug, which is that the function backing that method gets generated, because now it's in a module children list... We're going to need to look at the impl for Function too, and look into the signature to see if it's a constructor (we don't keep that around, but it's easy enough to do so).

emilio · 2017-07-19T23:04:16Z

tests/expectations/tests/issue-826-generating-methods-when-asked-not-to.rs

+}
+extern "C" {
+    #[link_name = "_ZN3Foo4testEv"]
+    pub fn Foo_test(this: *mut Foo);


This is the problem, actually... This is the function that is panicking on win32, which is unfortunate...

fitzgen · 2017-07-19T23:56:03Z

I'll have a fix tomorrow, but I'm calling it a day today.

fitzgen · 2017-07-20T18:12:10Z

r? @emilio

fitzgen · 2017-07-20T18:12:49Z

This found some existing bugs in gen-{con,de}structors-neg.hpp expectations!

fitzgen · 2017-07-20T18:15:34Z

src/ir/context.rs

                    }
                    ItemKind::Type(ref ty) => {
+                        if !cc.types {
+                            return false;
+                        }


Woops, I forgot to remove these checks; they're unnecessary now that there is a filter for is_enabled_for_codegen above.

emilio

Thanks for this fix! Neat as usual :)

emilio · 2017-07-20T19:41:02Z

src/ir/context.rs

@@ -1565,6 +1565,8 @@ impl<'ctx> BindgenContext<'ctx> {
        assert!(self.current_module == self.root_module);

        let roots = self.items()
+            // Only consider items that are enabled for codegen.
+            .filter(|&(_, item)| item.is_enabled_for_codegen(self))


(It may make sense to merge this with the filter below, your call)

I kind of like having it separate, because the latter filter's function is pretty big, and its nice to just separate it out and not worry about it anymore.

emilio · 2017-07-20T19:43:08Z

src/ir/traversal.rs

-impl TraversalPredicate for fn(Edge) -> bool {
-    fn should_follow(&self, edge: Edge) -> bool {
-        (*self)(edge)
+impl TraversalPredicate for for<'a> fn(&'a BindgenContext, Edge) -> bool {


Can't you just do:

impl<'a> TraversalPredicate for fn(&'a BindgenContext, Edge) -> bool { // ... }

... perhaps ... :-P

Oh. Huh. Apparently my subconscious was right this time:

$ cargo check Compiling bindgen v0.27.0 (file:///home/fitzgen/rust-bindgen) error[E0277]: the trait bound `fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool: ir::traversal::TraversalPredicate` is not satisfied --> src/ir/context.rs:179:5 | 179 | / traversal: ItemTraversal<'ctx, 180 | | 'gen, 181 | | ItemSet, 182 | | Vec<ItemId>, 183 | | for<'a> fn(&'a BindgenContext, Edge) -> bool>, | |__________________________________________________________________________^ the trait `ir::traversal::TraversalPredicate` is not implemented for `fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool` | = help: the following implementations were found: <fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool as ir::traversal::TraversalPredicate> = note: required by `ir::traversal::ItemTraversal` error[E0277]: the trait bound `fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool: ir::traversal::TraversalPredicate` is not satisfied --> src/ir/context.rs:777:5 | 777 | / fn assert_no_dangling_item_traversal<'me> 778 | | (&'me self) 779 | | -> traversal::AssertNoDanglingItemsTraversal<'me, 'ctx> { 780 | | assert!(self.in_codegen_phase()); ... | 786 | | traversal::all_edges) 787 | | } | |_____^ the trait `ir::traversal::TraversalPredicate` is not implemented for `fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool` | = help: the following implementations were found: <fn(&'a ir::context::BindgenContext<'_>, ir::traversal::Edge) -> bool as ir::traversal::TraversalPredicate> = note: required by `ir::traversal::ItemTraversal` error: aborting due to previous error(s) error: Could not compile `bindgen`. To learn more, run the command again with --verbose.

emilio · 2017-07-20T19:44:05Z

tests/expectations/tests/gen-destructors-neg.rs

@@ -21,7 +21,3 @@ fn bindgen_test_layout_Foo() {
                "Alignment of field: " , stringify ! ( Foo ) , "::" ,
                stringify ! ( bar ) ));
 }
-extern "C" {
-    #[link_name = "_ZN3FooD1Ev"]
-    pub fn Foo_Foo_destructor(this: *mut Foo);


bors-servo · 2017-07-20T19:58:48Z

☔ The latest upstream changes (presumably #824) made this pull request unmergeable. Please resolve the merge conflicts.

Windows uses non-ascii and non-visual characters in mangled names T.T

This makes only generating certain kinds of items more robust, since we don't need to keep checking whether codegen is enabled for different kinds of items all over the place, and can do it the once. It should also reduce the number of items we have to consider in our various analyses for which we don't ultimately care about the answer. Fixes rust-lang#826

fitzgen · 2017-07-20T21:52:11Z

@bors-servo r=emilio

bors-servo · 2017-07-20T21:52:11Z

📌 Commit 0b720a3 has been approved by emilio

bors-servo · 2017-07-20T21:52:14Z

⌛ Testing commit 0b720a3 with merge 9dbc2ca...

@upsuper

…not-to, r=emilio Only whitelist items for which we intend to generate bindings This means that we only need to check in one place, rather than at ever call site. This makes it so that we don't generate methods for the test case, but if I add the `--target` flag like in the original #826 test case, then I get a panic about an unknown ABI. I figure this change is worth landing now, and we can land the fix for the panic as a follow up. r? @upsuper

bors-servo · 2017-07-20T22:12:52Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing 9dbc2ca to master...

highfive assigned upsuper Jul 19, 2017

highfive added the S-awaiting-review label Jul 19, 2017

fitzgen mentioned this pull request Jul 19, 2017

Bindgen tries to generate bindings for methods even with --ignore-methods #826

Closed

fitzgen commented Jul 19, 2017

View reviewed changes

emilio requested changes Jul 19, 2017

View reviewed changes

fitzgen force-pushed the issue-826-generating-methods-when-asked-not-to branch from ef9136d to b392965 Compare July 20, 2017 18:11

fitzgen changed the title ~~Move codegen config checks into Codegen impls~~ Only whitelist items for which we intend to generate bindings Jul 20, 2017

highfive assigned emilio and unassigned upsuper Jul 20, 2017

fitzgen commented Jul 20, 2017

View reviewed changes

fitzgen force-pushed the issue-826-generating-methods-when-asked-not-to branch 2 times, most recently from 4a08371 to cda5e09 Compare July 20, 2017 18:33

emilio approved these changes Jul 20, 2017

View reviewed changes

highfive added the S-needs-rebase label Jul 20, 2017

fitzgen added 2 commits July 20, 2017 14:45

Escape mangled function names

b9b5648

Windows uses non-ascii and non-visual characters in mangled names T.T

fitzgen force-pushed the issue-826-generating-methods-when-asked-not-to branch from cda5e09 to 0b720a3 Compare July 20, 2017 21:51

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 20, 2017

bors-servo merged commit 0b720a3 into rust-lang:master Jul 20, 2017

highfive removed the S-awaiting-merge label Jul 20, 2017

bors-servo mentioned this pull request Jul 20, 2017

A bunch of little analysis cleanups #830

Merged

This was referenced Jul 21, 2017

Type parameter is incorrectly stripped when types are not generated #833

Closed

Behavior change for non-whitelisted types referenced by method #834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only whitelist items for which we intend to generate bindings #827

Only whitelist items for which we intend to generate bindings #827

fitzgen commented Jul 19, 2017

fitzgen Jul 19, 2017

emilio Jul 19, 2017

emilio left a comment

emilio Jul 19, 2017

fitzgen commented Jul 19, 2017

fitzgen commented Jul 20, 2017

fitzgen commented Jul 20, 2017

fitzgen Jul 20, 2017

emilio left a comment

emilio Jul 20, 2017

fitzgen Jul 20, 2017

emilio Jul 20, 2017

fitzgen Jul 20, 2017

fitzgen Jul 20, 2017

emilio Jul 20, 2017

bors-servo commented Jul 20, 2017

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

Only whitelist items for which we intend to generate bindings #827

Only whitelist items for which we intend to generate bindings #827

Conversation

fitzgen commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Jul 19, 2017

fitzgen commented Jul 20, 2017

fitzgen commented Jul 20, 2017

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jul 20, 2017

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017