-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Conversation
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, seems..ok
@oli-obk I wonder if we can make a test for that |
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 |
@bors r+ |
📌 Commit 2961937 has been approved by |
Use a `Set<T>` instead of a `Map<T, bool>` r? @nikomatsakis introduced in #44501
☀️ Test successful - status-appveyor, status-travis |
r? @nikomatsakis
introduced in #44501