Skip to content

/rust-bindgen

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 --no-partialeq <regex> flag #996

Merged
merged 1 commit into from Sep 20, 2017

Conversation

Reviewers

@fitzgen fitzgen

Assignees

@fitzgen fitzgen

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@alexeyzab @bors-servo @fitzgen @highfive
@alexeyzab
Copy link
Contributor

alexeyzab commented Sep 16, 2017 •
edited

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

@highfive highfive assigned fitzgen Sep 16, 2017

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

@fitzgen

fitzgen reviewed Sep 18, 2017
View changes

Copy link
Member

fitzgen left a comment

Yep! This looks like what I would expect it to. You're on the right path :)
src/ir/analysis/derive_partial_eq.rs Outdated
@@ -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) {

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Sep 18, 2017

Member

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 18, 2017

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

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

@alexeyzab alexeyzab force-pushed the alexeyzab:add-no-partialeq-command branch from 7c8f70b to e838a12 Sep 19, 2017

@alexeyzab

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

alexeyzab commented Sep 19, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 19, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 19, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 19, 2017

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

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

@alexeyzab

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

alexeyzab commented Sep 19, 2017 •
edited

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! :)

@alexeyzab alexeyzab force-pushed the alexeyzab:add-no-partialeq-command branch from 067c57b to 2bcf255 Sep 19, 2017

@alexeyzab
Add --no-partialeq <regex> flag 
- [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 #965
Verified
This commit was signed with a verified signature.
@alexeyzab alexeyzab Alexey Zabelin
GPG key ID: 3A169985478324E9 Learn about signing commits
Loading status checks…
ec8456b

@alexeyzab alexeyzab force-pushed the alexeyzab:add-no-partialeq-command branch from a6eadb4 to ec8456b Sep 19, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Sep 19, 2017

@bors-servo r+

Thanks @alexeyzab !
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 19, 2017

📌 Commit ec8456b has been approved by fitzgen

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

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 19, 2017

⌛️ Testing commit ec8456b with merge 4842973...

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

bors-servo added a commit that referenced this pull request Sep 19, 2017

@bors-servo
Auto merge of #996 - alexeyzab:add-no-partialeq-command, r=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
Loading status checks…
4842973
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 20, 2017

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

@bors-servo bors-servo merged commit ec8456b into rust-lang:master Sep 20, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

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

@fitzgen fitzgen referenced this pull request Sep 21, 2017

Open

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

0 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.