-
Notifications
You must be signed in to change notification settings - Fork 243
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
Remove duplicate remove_outer_deref
s and refactor mod deref
#573
base: tests.refs.fields
Are you sure you want to change the base?
Remove duplicate remove_outer_deref
s and refactor mod deref
#573
Conversation
…in terms of `try_remove_outer_deref_as_ref` so we know they're in sync.
…and this lets us eliminate duplicate calls.
remove_outer_deref
s and refactor mod deref
…ng `try_remove_outer_deref`.
It encouraged an anti-pattern anyways, using `has_outer_deref` and then `remove_outer_deref` instead of `try_remove_outer_deref`.
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 in favor of combining the match arms. I do however like your original idea outlined here in order to bind source
in the match arm for use in the body and would support that change.
Rvalue::AddressOf(_, p) => { | ||
// Instrument which local's address is taken | ||
self.loc(location.successor_within_block(), addr_local_fn) | ||
let source = try_remove_outer_deref(*p, self.tcx()) |
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 don't want to combine these, there are more complicated cases incoming and it'll be much clearer if they're separate
// this is a reborrow or field reference, i.e. _2 = &(*_1) | ||
let source = remove_outer_deref(*p, self.tcx()); | ||
if let BorrowKind::Mut { .. } = bkind { | ||
let source = try_remove_outer_deref(*p, self.tcx()); |
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 don't want to combine these, there are more complicated cases incoming and it'll be much clearer if they're separate
My original idea uses |
|
I understand there might be more complex logic coming, so I shouldn't combine the logic in the way I did, but I don't think combining |
This refactors
mod deref
, implementing most of the functions there in terms of onetry_remove_outer_deref_as_ref
so that we know they're all in sync. Also,try_remove_outer_deref
is added so that you don't have to do separatehas_outer_deref
andremove_outer_deref
calls.Then I used this to de-duplicate the 1
has_outer_ref
and 2remove_outer_deref
calls in theRValue::{AddressOf,Ref}
match
arms by combining the arms and usingtry_remove_outer_deref
.Then I used
apply
to simplify the conditional builder logic. It's quite a small but useful dependency for functional, chaining code.The remaining code is still quite similar between the
RValue::AddressOf
andRvalue::Ref
match
arms, so maybe we can refactor that into common code as well.