-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
NLL fixes #46984
NLL fixes #46984
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,6 +149,18 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD> | |
fn reconstruct_statement_effect(&mut self, loc: Location) { | ||
self.stmt_gen.reset_to_empty(); | ||
self.stmt_kill.reset_to_empty(); | ||
{ | ||
let mut sets = BlockSets { | ||
on_entry: &mut self.curr_state, | ||
gen_set: &mut self.stmt_gen, | ||
kill_set: &mut self.stmt_kill, | ||
}; | ||
self.base_results | ||
.operator() | ||
.before_statement_effect(&mut sets, loc); | ||
} | ||
self.apply_local_effect(loc); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, so, I have the edge-effect implemented, but I'm not sure what I think about it. On the one hand, I think the work I did feels like a cleaner approach than the existing "special case" for "call-normal-return". On the other hand, adding edge effects is not quite sufficient to make everything work correctly: the problem has to do with how the "activation" checks in two-phase borrows work. What I did on my branch is basically that the "statement effect" for some statement S kills all the borrows whose regions do not include the successor S'. In other words, when you check a statement like This works fine for most things, but it has a funny effect on the activation check. In particular, to determine at some statement S which borrows have been activated, we iterate through the gens that are present in the statement effect. But this isn't quite right: the statement effect here represents the state on entry to S', but we really want to see some point before that -- the statement before as we exit S, but before we enter S'. In particular, if you have some borrow that is activated by S, but goes out of scope in S', we want to see that activation as a "gen", but the setup I described above will gen the bit but then later kill it. I can see two ways to fix this. The first is to allow gen/kill bits to both be set. In the specific case of activations, this would mean that statement S both gens and kills the same bit. This seems ok but only happens to work because in this case the kill comes temporally after the gen. The other was adding a method like the one you wound up adding in this PR. And maybe that just suffices, then. My only nit here is that I'd like to improve the comments on
My mental model then is that there are sort of two points in time for each statement, let's call them S-before and S-after. It's probably worth noting somewhere (or maybe even with a debug-assertion) that there is an implicit cursor -- i.e., you can't reconstruct the statement effect for statement 1 if you have not done statement 0 first. (In any case, we could take the edge effect changes just as a refactoring, but we don't have to. It makes the code more regular but perhaps a bit more complex. I can open the PR and @pnkfelix and I can discuss.) |
||
|
||
let mut sets = BlockSets { | ||
on_entry: &mut self.curr_state, | ||
gen_set: &mut self.stmt_gen, | ||
|
@@ -162,6 +174,18 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD> | |
fn reconstruct_terminator_effect(&mut self, loc: Location) { | ||
self.stmt_gen.reset_to_empty(); | ||
self.stmt_kill.reset_to_empty(); | ||
{ | ||
let mut sets = BlockSets { | ||
on_entry: &mut self.curr_state, | ||
gen_set: &mut self.stmt_gen, | ||
kill_set: &mut self.stmt_kill, | ||
}; | ||
self.base_results | ||
.operator() | ||
.before_terminator_effect(&mut sets, loc); | ||
} | ||
self.apply_local_effect(loc); | ||
|
||
let mut sets = BlockSets { | ||
on_entry: &mut self.curr_state, | ||
gen_set: &mut self.stmt_gen, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
#![feature(nll)] | ||
|
||
// run-pass | ||
|
||
fn vec() { | ||
let mut _x = vec!['c']; | ||
let _y = &_x; | ||
_x = Vec::new(); | ||
} | ||
|
||
fn int() { | ||
let mut _x = 5; | ||
let _y = &_x; | ||
_x = 7; | ||
} | ||
|
||
fn main() { | ||
vec(); | ||
int(); | ||
} |
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.
is the change here the inclusion of
Box
? (i.e., the behavior for other cases is the same?)