-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Inspect parents paths when checking for moves #54255
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
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.
Looks good. Left some requests for comments. :)
let mut mpis = vec![mpi]; | ||
|
||
let mut curr_mpi = mpi; | ||
while let Some(parent_mpi) = &self.move_data.move_paths[curr_mpi].parent { |
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.
you probably don't need the &
here or the *
on the lines below
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'd also like to see a comment explaining this. Something like:
If we are found a use of a.b.c
which was in error, then we want to look for moves not only of a.b.c
but also a.b
and a
.
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.
Writing that out makes me realize that probably there is a similar bug the other way... e.g., what happens here?
fn main() {
let x = (vec![1, 2, 3], );
drop(x.0);
drop(x);
}
Ah, trying it on play, it seems to work ok. Interesting. I guess that the moves
data already handles this case -- i.e., a move of x.0
is marked as a move of x
. Maybe add this to the comment:
Note that the moves data already includes "parent" paths, so we don't have to worry about the other case: that is, if there is a move of a.b.c
, it is already marked as a move of a.b
and a
as well, so we will generate the correct errors there.
but @pnkfelix can you verify that comment? ☝️
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.
Also, I wonder if we want to make a helper for iterating over all parents...
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.
@nikomatsakis everything is fixed.
1d08ffe
to
504d732
Compare
504d732
to
e9029ce
Compare
@bors r+ |
📌 Commit e9029ce has been approved by |
…sakis Inspect parents paths when checking for moves Closes #52669
☀️ Test successful - status-appveyor, status-travis |
Closes #52669