[WIP] Take 2: change how MIR represents Place#54426
[WIP] Take 2: change how MIR represents Place#54426csmoe wants to merge 34 commits intorust-lang:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
22099d7 to
3cfa0bc
Compare
src/librustc/mir/tcx.rs
Outdated
There was a problem hiding this comment.
I find this operation dishearteningly inefficient :(.
There was a problem hiding this comment.
If we don't "re-intern" the slice of elements, then in what way is it inefficient?
I think it'd be nice to be able to treat a NeoPlace as either a list or a tree as you choose, and this operation allows for that (although I would probably tweak the signature; I'd prefer it returns something like "Result<(NeoPlace, ProjectionElem), Base>`. ie., either you reached the base case, or else not.
There was a problem hiding this comment.
Maybe something like
enum NeoPlaceTree {
Leaf(PlaceBase),
Branch(NeoPlace<'tcx>, ProjectionElem<'tcx>),
}and into_tree(self) -> NeoPlaceTree as the signature (lifetimes elided, clearly).
src/librustc/mir/tcx.rs
Outdated
There was a problem hiding this comment.
You can rewrite this as going backwards in the projection list.
If you want, you can use let mut elems = self.elems.iter().rev(); and then call next() on that once, check whether it's Deref, and if it is, call next() again.
You can also use indices.
There was a problem hiding this comment.
It feels like this is a case where treating the projection as a tree (sort of what this code is doing) is a bit cleaner. In particular, we want to look for something like P.f or *P.f and then test the type of P. We can certainly "walk backwards" over the elements, but once we've peeled off the * and the .f we sitll kind of want to extract the P as a unit, right?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c18a525 to
0e689b1
Compare
src/librustc/mir/tcx.rs
Outdated
There was a problem hiding this comment.
There is no need to intern again here; just use place_elems directly. It is already pointing into interned memory, right?
8e4e8aa to
aee5eae
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
606860a to
f0ac652
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #58254) made this pull request unmergeable. Please resolve the merge conflicts. |
|
After conversations about MIR 2.0, I think this PR doesn't make sense anymore as is. In any case this is being discussed in Zulip. |
Closes #52708
Introduce the new
Placewith "incremental refactoring" instead of the previous approach which is hard to maintain.r? @nikomatsakis