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

Use a Set<T> instead of a Map<T, bool> #45736

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 3, 2017

r? @nikomatsakis

introduced in #44501

@@ -79,12 +79,12 @@ fn const_is_rvalue_promotable_to_static<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
.expect("rvalue_promotable_map invoked with non-local def-id");
let body_id = tcx.hir.body_owned_by(node_id);
let body_hir_id = tcx.hir.node_to_hir_id(body_id.node_id);
tcx.rvalue_promotable_map(def_id).contains_key(&body_hir_id.local_id)
tcx.rvalue_promotable_map(def_id).contains(&body_hir_id.local_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this exact thing in clippy, and it was very wrong. I'm fairly certain it was wrong here, too.

This checked whether the local_id was ever checked for rvalue promotion. Not whether it is rvalue promotable.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -897,7 +897,7 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
expr_ty: Ty<'tcx>)
-> cmt<'tcx> {
let hir_id = self.tcx.hir.node_to_hir_id(id);
let promotable = self.rvalue_promotable_map.as_ref().map(|m| m[&hir_id.local_id])
let promotable = self.rvalue_promotable_map.as_ref().map(|m| m.contains(&hir_id.local_id))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change in semantics. We do not panic anymore if local_id was never checked for rvalue promotion, we just return false

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, seems..ok

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2017
@nikomatsakis
Copy link
Contributor

@oli-obk I wonder if we can make a test for that contains_key call -- I agree it was wrong, interesting that it didn't break anything.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 3, 2017

I wondered the same thing, but found it very hard to actually do. There are too many other checks, it was probably redundant anyway, since it would additionally have to go through trans const eval

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 6, 2017

📌 Commit 2961937 has been approved by nikomatsakis

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017
@bors
Copy link
Contributor

bors commented Nov 9, 2017

⌛ Testing commit 2961937 with merge fd9ecfd...

bors added a commit that referenced this pull request Nov 9, 2017
Use a `Set<T>` instead of a `Map<T, bool>`

r? @nikomatsakis

introduced in #44501
@bors
Copy link
Contributor

bors commented Nov 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing fd9ecfd to master...

@bors bors merged commit 2961937 into rust-lang:master Nov 9, 2017
@oli-obk oli-obk deleted the rvalue_promotable_map branch December 8, 2017 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants