-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Miscellaneous cleanups to borrowck related code #150550
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
base: main
Are you sure you want to change the base?
Conversation
| let (idx, _) = self.location_map.insert_full(location, borrow); | ||
| let idx = BorrowIndex::from(idx); | ||
|
|
||
| self.insert_as_pending_if_two_phase(location, assigned_place, kind, idx); |
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 felt very confusing to read to me. we first insert the borrow with the wrong activation location (we assume its not two phase). and then insert_as_pending_if_two_phase does more than it says and actually fixes up the activation location to what it should have been from the start
I inlined this function as it only had one call site and deferred inserting into the location map until we actually choose a TwoPhaseActivation to use.
| /// Returns whether borrows represented by this kind are allowed to be split into separate | ||
| /// Reservation and Activation phases. | ||
| pub fn allows_two_phase_borrow(&self) -> bool { | ||
| pub fn is_two_phase_borrow(&self) -> bool { |
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.
in some sense its always a two phase borrow, just sometimes its activated at the same point its created I think?
The previous name confused me because it made it sound like a reference may or may not be a two phase borrow and then we went on to unconditionally treat it as two phase without ever checking if it "actually is".
| constraints.push(c) | ||
| } | ||
|
|
||
| // Currently `implied_outlives_bounds` will normalize the provided |
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'm not sure how useful a lot of my comments here actually are given afaik @tiif is rewriting the logic for computing implies bounds so this code might just all get yoten soon:tm:
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 think the core logic here won't change too much, so it will still be very useful :3
r? lcnr