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

Simplify feature-handling code #3742

Merged
merged 1 commit into from
Feb 25, 2017
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Feb 21, 2017

A neat (imo :) ) hack to use an empty &HashSet instead of Option<&HashSet>.

@rust-highfive
Copy link

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -74,6 +75,7 @@ mod encode;
pub struct Resolve {
graph: Graph<PackageId>,
replacements: HashMap<PackageId, PackageId>,
empty_features: HashSet<String>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible to make this a function-local lazy-static I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah I prefer this, thanks!

@@ -385,7 +375,7 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
rustc: util::hash_u64(&cx.config.rustc()?.verbose_version),
target: util::hash_u64(&unit.target),
profile: util::hash_u64(&unit.profile),
features: format!("{:?}", features),
features: format!("{:?}", cx.resolve.features_sorted(unit.pkg.package_id())),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only place where behavior is changed, because we don't print Some, None. But fingerprints can change between cargo releases, can't they?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah yeah this is totally fine

@matklad
Copy link
Member Author

matklad commented Feb 21, 2017

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

Awesome cleanup!

@bors
Copy link
Collaborator

bors commented Feb 21, 2017

📌 Commit 2af1c97 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 22, 2017

⌛ Testing commit 2af1c97 with merge 4716ef8...

@bors
Copy link
Collaborator

bors commented Feb 22, 2017

💔 Test failed - status-travis

@matklad
Copy link
Member Author

matklad commented Feb 22, 2017

Yay, the not rocket science rule has prevented a broken build! Rebased and fixed.

@matklad
Copy link
Member Author

matklad commented Feb 22, 2017

Quick search showed a handful of instances where we use Option<&Collection>, so it may be a nice thing to do to add struct DefaultMap<K, V: Default> { default: V = V::default(); inner: HashMap<K, V> } to utils.

@alexcrichton
Copy link
Member

@bors: r+

Yeah I suspect Cargo'd make good use of that

@bors
Copy link
Collaborator

bors commented Feb 22, 2017

📌 Commit 3267ca5 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 22, 2017

🔒 Merge conflict

@bors
Copy link
Collaborator

bors commented Feb 22, 2017

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

@matklad
Copy link
Member Author

matklad commented Feb 25, 2017

@alexcrichton could you r+ rebased code? :)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 25, 2017

📌 Commit 50f1c17 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 25, 2017

⌛ Testing commit 50f1c17 with merge 163de44...

bors added a commit that referenced this pull request Feb 25, 2017
Simplify feature-handling code

A neat (imo :) ) hack to use an empty `&HashSet` instead of `Option<&HashSet>`.
@bors
Copy link
Collaborator

bors commented Feb 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 163de44 to master...

@bors bors merged commit 50f1c17 into rust-lang:master Feb 25, 2017
@matklad matklad deleted the sets-are-monoid branch March 27, 2017 10:04
@ehuss ehuss added this to the 1.17.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants