Add --no-partialeq <regex> flag #996

alexeyzab · 2017-09-16T22:49:28Z

Related to #965.

Add a new RegexSet member to bindgen::Builder (similar to the whitelisted_types set).
A Builder method to add strings to that RegexSet.
Plumbing in src/options.rs to convert --no-partialeq CLI flags into invocations of the builder method.
Make the MonotoneFramework::constrain function in src/ir/analysis/derive_partialeq.rs check if the given item is explicitly marked not to be Partialeq, and if so, insert it into the self.cannot_derive_partialeq set via return self.insert(id).
Tests!
When the no-partialeq type is transitively referenced by a whitelisted item
When the no-partialeq type is explicitly whitelisted
When the no-partialeq type is marked opaque

This is my first pass at implementing this functionality, I haven't implemented the tests yet. I wanted to make sure I am on the right track, particularly when it comes to updating MonotoneFramework::constrain.

r? @fitzgen

fitzgen

Yep! This looks like what I would expect it to. You're on the right path :)

fitzgen · 2017-09-18T17:23:58Z

src/ir/analysis/derive_partial_eq.rs

@@ -144,6 +145,11 @@ impl<'ctx> MonotoneFramework for CannotDerivePartialEq<'ctx> {
            };
        }

+        let name = item.canonical_name(self.ctx);
+        if self.ctx.options().no_partialeq_types.matches(&name) {


Let's abstract this check into a BindgenContext::no_partialeq_by_name, similar to BindgenContext::is_opaque_by_name and BindgenContext::is_blacklisted_by_name.

bors-servo · 2017-09-18T21:25:04Z

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

alexeyzab · 2017-09-19T16:05:18Z

r? @fitzgen
I've added the required tests, but I am not quite sure I did it right. I use --with-derive-partialeq for all three tests and then add the additional flags per your description. When I explicitly whitelist or make opaque a particular type PartialEq still gets derived.

fitzgen · 2017-09-19T17:32:15Z

It makes sense that opaque types still derive(PartialEq) because the no_partialeq_by_name check comes after the opaque check in the constrain function. We should move the no_partialeq_by_name above the opaque check.

The explicitly whitelisted types always getting derive(PartialEq) even when we request they shouldn't sounds more problematic. Let me look at the code a little more...

fitzgen · 2017-09-19T17:40:25Z

Hm yeah the test headers look how I'd expect them to, but the generated bindings don't. I'd try adding some debug println!s in the CanDerivePartialEq implementations and lookup_item_id_can_derive_partialeq methods. Then, I'd run with extra logging:

$ RUST_LOG=bindgen::ir::analysis RUST_LOG_LEVEL=trace ./tests/test-one.sh <one of your new tests>

If you still can't figure out what's going on after that, I can try pulling these changes down myself and investigating.

BTW, I'm not sure if you're aware that the "impl period" has just begun, but the folks hacking on bindgen during the impl period are hanging out in https://gitter.im/rust-impl-period/WG-dev-tools-bindgen if you want to join us :)

bors-servo · 2017-09-19T18:20:36Z

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

alexeyzab · 2017-09-19T19:25:42Z

We should move the no_partialeq_by_name above the opaque check.

I've tried it just now, but the result is still the same. Makes me wonder if no_partialeq_by_name is doing the right thing at all. I'll try to figure it out.

Thanks for the invite! :)

- [x] Add a new RegexSet member to bindgen::Builder (similar to the whitelisted_types set). - [x] A Builder method to add strings to that RegexSet. - [x] Plumbing in src/options.rs to convert --no-partialeq <regex> CLI flags into invocations of the builder method. - [x] Make the MonotoneFramework::constrain function in src/ir/analysis/derive_partialeq.rs check if the given item is explicitly marked not to be Partialeq, and if so, insert it into the self.cannot_derive_partialeq set via return self.insert(id). - [x] Tests! - [x] When the no-partialeq type is transitively referenced by a whitelisted item - [x] When the no-partialeq type is explicitly whitelisted - [x] When the no-partialeq type is marked opaque Fixes rust-lang#965

fitzgen · 2017-09-19T23:48:41Z

@bors-servo r+

Thanks @alexeyzab !

bors-servo · 2017-09-19T23:48:42Z

📌 Commit ec8456b has been approved by fitzgen

bors-servo · 2017-09-19T23:48:44Z

⌛ Testing commit ec8456b with merge 4842973...

@fitzgen

Add --no-partialeq <regex> flag Related to #965. - [x] Add a new RegexSet member to bindgen::Builder (similar to the whitelisted_types set). - [x] A Builder method to add strings to that RegexSet. - [x] Plumbing in src/options.rs to convert --no-partialeq <regex> CLI flags into invocations of the builder method. - [x] Make the MonotoneFramework::constrain function in src/ir/analysis/derive_partialeq.rs check if the given item is explicitly marked not to be Partialeq, and if so, insert it into the self.cannot_derive_partialeq set via return self.insert(id). - [x] Tests! - [x] When the no-partialeq type is transitively referenced by a whitelisted item - [x] When the no-partialeq type is explicitly whitelisted - [x] When the no-partialeq type is marked opaque This is my first pass at implementing this functionality, I haven't implemented the tests yet. I wanted to make sure I am on the right track, particularly when it comes to updating `MonotoneFramework::constrain`. r? @fitzgen

bors-servo · 2017-09-20T00:29:28Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 4842973 to master...

highfive assigned fitzgen Sep 16, 2017

highfive added the S-awaiting-review label Sep 16, 2017

fitzgen reviewed Sep 18, 2017

View reviewed changes

highfive added the S-needs-rebase label Sep 18, 2017

alexeyzab force-pushed the add-no-partialeq-command branch from 7c8f70b to e838a12 Compare September 19, 2017 16:01

highfive added the S-needs-rebase label Sep 19, 2017

alexeyzab force-pushed the add-no-partialeq-command branch from 067c57b to 2bcf255 Compare September 19, 2017 20:50

alexeyzab force-pushed the add-no-partialeq-command branch from a6eadb4 to ec8456b Compare September 19, 2017 23:43

highfive removed S-awaiting-review S-needs-rebase labels Sep 19, 2017

highfive added the S-awaiting-merge label Sep 19, 2017

bors-servo merged commit ec8456b into rust-lang:master Sep 20, 2017

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

fitzgen mentioned this pull request Sep 21, 2017

Allow specifying certain types we shouldn't derive/impl Default for #963

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --no-partialeq <regex> flag #996

Add --no-partialeq <regex> flag #996

alexeyzab commented Sep 16, 2017 •

edited

Loading

fitzgen left a comment

fitzgen Sep 18, 2017

bors-servo commented Sep 18, 2017

alexeyzab commented Sep 19, 2017

fitzgen commented Sep 19, 2017

fitzgen commented Sep 19, 2017

bors-servo commented Sep 19, 2017

alexeyzab commented Sep 19, 2017 •

edited

Loading

fitzgen commented Sep 19, 2017

bors-servo commented Sep 19, 2017

bors-servo commented Sep 19, 2017

bors-servo commented Sep 20, 2017

Add --no-partialeq <regex> flag #996

Add --no-partialeq <regex> flag #996

Conversation

alexeyzab commented Sep 16, 2017 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Sep 18, 2017

Choose a reason for hiding this comment

bors-servo commented Sep 18, 2017

alexeyzab commented Sep 19, 2017

fitzgen commented Sep 19, 2017

fitzgen commented Sep 19, 2017

bors-servo commented Sep 19, 2017

alexeyzab commented Sep 19, 2017 • edited Loading

fitzgen commented Sep 19, 2017

bors-servo commented Sep 19, 2017

bors-servo commented Sep 19, 2017

bors-servo commented Sep 20, 2017

alexeyzab commented Sep 16, 2017 •

edited

Loading

alexeyzab commented Sep 19, 2017 •

edited

Loading