Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Reuse the `DefsUsesVisitor` in `simulate_block()`. #51870
Conversation
|
Looks good, left a "cleanup nit" |
| statement_defs_uses.apply(&mut bits); | ||
| visitor.defs_uses.clear(); | ||
| statement.apply(statement_location, &mut visitor); | ||
| visitor.defs_uses.apply(&mut bits); |
nikomatsakis
Jun 28, 2018
Contributor
Hmm. It'd be nice to encapsulate the "clear, apply, etc" logic into some helper so as to make this more obvious. Since we only want to clear the bits before each statement, though, perhaps some setup like this would be good:
impl DefsUseVisitor {
fn clear_then_update_bits(&mut self, location: Location, value: &impl MirVisitable<'tcx>, bits: &mut LocalSet) {
self.clear();
self.update_bits(location, value, bits);
}
/// Update `bits` with the affects of `value`. We should always visit in reverse order.
/// This method assumes that we have not visited anything before; if you have, use `clear_and_update_bits`.
fn update_bits(&mut self, location: Location, value: &impl MirVisitable<'tcx>, bits: &mut LocalSet) {
value.apply(location, self);
self.defs_uses.apply(bits);
}
}
What do you think?
Hmm. It'd be nice to encapsulate the "clear, apply, etc" logic into some helper so as to make this more obvious. Since we only want to clear the bits before each statement, though, perhaps some setup like this would be good:
impl DefsUseVisitor {
fn clear_then_update_bits(&mut self, location: Location, value: &impl MirVisitable<'tcx>, bits: &mut LocalSet) {
self.clear();
self.update_bits(location, value, bits);
}
/// Update `bits` with the affects of `value`. We should always visit in reverse order.
/// This method assumes that we have not visited anything before; if you have, use `clear_and_update_bits`.
fn update_bits(&mut self, location: Location, value: &impl MirVisitable<'tcx>, bits: &mut LocalSet) {
value.apply(location, self);
self.defs_uses.apply(bits);
}
}What do you think?
nnethercote
Jun 28, 2018
Author
Contributor
TBH, think it's less clear, and it's more code, and those factors outweigh the benefit of not repeating. But I'll do it if that's what is necessary for an r+ :)
TBH, think it's less clear, and it's more code, and those factors outweigh the benefit of not repeating. But I'll do it if that's what is necessary for an r+ :)
nikomatsakis
Jun 29, 2018
Contributor
Yes, it would please me — not necessarily those exact names and setup, but some kind of method that groups together the logical operation that is occuring here ("clear flag, do visit, apply the result"), and ideally some comments about what the heck is going on.
-- Your capricious reviewer
Yes, it would please me — not necessarily those exact names and setup, but some kind of method that groups together the logical operation that is occuring here ("clear flag, do visit, apply the result"), and ideally some comments about what the heck is going on.
-- Your capricious reviewer
This avoids a bunch of allocations for the bitsets within it, speeding up a number of NLL benchmarks, the best by 1%.
nnethercote
force-pushed the
nnethercote:reuse-DefsUsesVisitor
branch
from
02d85e2
to
b0c7812
|
New, super-DRY version. |
|
@bors r+ -- thanks @nnethercote ! |
|
|
Reuse the `DefsUsesVisitor` in `simulate_block()`. This avoids a bunch of allocations for the bitsets within it, speeding up a number of NLL benchmarks, the best by 1%. r? @nikomatsakis
|
|
This avoids a bunch of allocations for the bitsets within it,
speeding up a number of NLL benchmarks, the best by 1%.
r? @nikomatsakis