-
Notifications
You must be signed in to change notification settings - Fork 248
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
analyze: move Option<&mut T>
on last use instead of reborrowing
#1179
Conversation
set: HashMap::new(), | ||
}; | ||
for (bb, actions) in block_actions.iter_enumerated() { | ||
let mut live = BitSet::new_empty(mir.local_decls.len()); |
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.
Minor nit: this loop body looks very similar to the previous one (you're computing the same live
information but inserting into last_use
as an additional step). Could you de-duplicate them, maybe with a helper function or a closure?
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.
The amount of duplicated code is small enough that I think it's fine to leave it as-is. The tricky logic of mapping MIR statements to defs and uses of locals is already factored out in the building of the block_actions
.
for sl in &self.sub_loc { | ||
match *sl { | ||
SubLoc::Dest => { | ||
which = Some(last_use::WhichPlace::Lvalue); |
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.
Can there be multiple sub_loc
s that set which
in this loop? Do you always want the last one?
If you only need one, you could exit early with return
.
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.
Added a comment in 8f40e87 - there can indeed be multiple matching sub_loc
s, and we want the last one.
/// suitable to be used as the result expression of a function. | ||
can_move: bool, | ||
/// If set, the cast builder will emit a downgrade/borrow operation even for no-op casts, if | ||
/// the thing being cast can't be moved (`!can_move`) and also can't be copied. |
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.
Are there only 3 states between the 2 variables: can-move, can-copy, must-borrow? Maybe using an enum
here would be better.
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.
There are indeed only three possible behaviors - can_move && borrow
produces the same cast as can_move && !borrow
. However, the code that sets the flags handles the two independently (and in particular, sometimes produces the can_move && borrow
case). So I think it's better to leave the code as-is, with the logic mapping the four cases to three outcomes centralized in the builder.
839571e
to
ab616f3
Compare
a9e7256
to
402166a
Compare
28cfbd4
to
ba95182
Compare
402166a
to
78c32c5
Compare
ba95182
to
8f40e87
Compare
Currently, we reborrow
Option<&mut T>
at most use sites by callingp.as_deref_mut()
. This creates a new reference that's tied to the lifetime of theOption
, rather than the lifetime of the original reference. This is fine for temporaries and local variables, but creates a problem when returning the result from the current function:This produces a borrowck error because
as_deref_mut
has the signature(&'b mut Option<&'a mut T>) -> Option<&'b mut T>
, meaning the result must not outlivex
.The fix here is to consume
x
:In this case,
map
has the signature(Option<&'a mut T>, [closure]) -> Option<&'a mut i32>
, preserving the lifetime of the input reference.This branch adds an analysis to identify the "last use" of each local variable and modifies the rewriting rules around
Option<T>
to omit.as_deref_mut()
at the last use.