Stream: t-compiler/wg-mir-opt

Topic: Place 2.0


Santiago Pastorino (Mar 19 2019 at 17:59, on Zulip):

@oli you around?

Santiago Pastorino (Mar 19 2019 at 18:00, on Zulip):

btw link to previous discussion

Santiago Pastorino (Mar 19 2019 at 18:01, on Zulip):

saw this https://github.com/rust-lang/rust/pull/59232 and I guess we are going to have a lot of conflicts between this one and the Place struct thing

Santiago Pastorino (Mar 19 2019 at 18:01, on Zulip):

wondering what should we do exactly?

oli (Mar 19 2019 at 18:02, on Zulip):

i'll be here in 30 mins

Santiago Pastorino (Mar 19 2019 at 18:03, on Zulip):

:+1:

oli (Mar 19 2019 at 18:25, on Zulip):

@Santiago Pastorino I think the conflicts are going to be benign, but yes, they are definitely going to happen

Santiago Pastorino (Mar 19 2019 at 18:30, on Zulip):

ok

Santiago Pastorino (Apr 04 2019 at 19:21, on Zulip):

hi @oli back to OSS life :)

Santiago Pastorino (Apr 04 2019 at 19:22, on Zulip):

my idea is to do the struct change now

Santiago Pastorino (Apr 04 2019 at 19:22, on Zulip):

and then do the iterator thing, which I opened a WIP some time ago

Santiago Pastorino (Apr 04 2019 at 19:22, on Zulip):

does that sound correct?

Santiago Pastorino (Apr 04 2019 at 19:22, on Zulip):

I need to start remembering what I was doing :P

Santiago Pastorino (Apr 04 2019 at 20:18, on Zulip):

@oli @eddyb for the sake of understanding a bit why is this question there https://github.com/rust-lang/rust/pull/59232/files#r269485022 ?

eddyb (Apr 04 2019 at 20:21, on Zulip):

oh hey I was gonna ask you what the status of this refactor is, but I couldn't find you on here because Zulip makes people use their full names (which is IMO silly for a chat platform like this but nevermind)

eddyb (Apr 04 2019 at 20:21, on Zulip):

that question is regarding the ty field

eddyb (Apr 04 2019 at 20:21, on Zulip):

those Tys (there's another one on field projections) are there for performance reasons, as I understand it

eddyb (Apr 04 2019 at 20:21, on Zulip):

since you can always get the type if you have a TyCtxt

Matthew Jasper (Apr 04 2019 at 20:23, on Zulip):

NLL uses them to store a version with all of the lifetimes replaced with inference variables. I'm not sure how necessary this is.

eddyb (Apr 04 2019 at 20:24, on Zulip):

you never need that

eddyb (Apr 04 2019 at 20:24, on Zulip):

static can't be parameterized

eddyb (Apr 04 2019 at 20:24, on Zulip):

and for fields, the type is the one you get by substituting the definition type with the substitiions of the ADT

eddyb (Apr 04 2019 at 20:24, on Zulip):

NLL replaces the lifetimes in those substitutions, so they would naturally propagate to the field type

eddyb (Apr 04 2019 at 20:25, on Zulip):

like, if you think about it, lifetimes have to come from somewhere, those Tys are just caches

Santiago Pastorino (Apr 04 2019 at 20:29, on Zulip):

oh hey I was gonna ask you what the status of this refactor is, but I couldn't find you on here because Zulip makes people use their full names (which is IMO silly for a chat platform like this but nevermind)

I was not working on this thing because I was running Rust Latam

Santiago Pastorino (Apr 04 2019 at 20:29, on Zulip):

back to this :)

Santiago Pastorino (Apr 04 2019 at 20:30, on Zulip):

tomorrow I'm going to start spending more time on time and the first couple of changes should be fairly easy to do I think

Santiago Pastorino (Apr 05 2019 at 15:08, on Zulip):

@oli you around?

Santiago Pastorino (Apr 05 2019 at 15:09, on Zulip):

I was wondering in this code ...

Santiago Pastorino (Apr 05 2019 at 15:09, on Zulip):
struct Place {
    base: PlaceBase,
    projection: PlaceProjection,
}
enum PlaceProjection {
    Base,
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}
Santiago Pastorino (Apr 05 2019 at 15:09, on Zulip):

what do we want to do with the generic definition we previously had of the Projection

Santiago Pastorino (Apr 05 2019 at 15:09, on Zulip):

I guess if we want PlaceProjection that's not generic anymore

Santiago Pastorino (Apr 05 2019 at 15:10, on Zulip):

but I guess we want to have some generic definition?

Santiago Pastorino (Apr 05 2019 at 15:10, on Zulip):

maybe something like ...

Santiago Pastorino (Apr 05 2019 at 15:12, on Zulip):
struct Place {
    base: PlaceBase,
    projection: PlaceProjection,
}
pub type PlaceProjection = GenericProjection<Place, Ty>;
enum GenericProjection<V, T> {
    Base,
    Projection(Box<GenericProjection<V, T>>),
    Deref,
    Index(..),
    ...
}
Santiago Pastorino (Apr 05 2019 at 15:12, on Zulip):

I guess we want something like that?

Santiago Pastorino (Apr 05 2019 at 16:12, on Zulip):

@eddyb thoughts? ^^^

Santiago Pastorino (Apr 05 2019 at 16:13, on Zulip):

anyway, I'm working on it regardless, asking just in case you want to do something different than I'm doing :)

eddyb (Apr 06 2019 at 11:14, on Zulip):

you should keep it as generic as possible, I think

eddyb (Apr 06 2019 at 11:14, on Zulip):

and as aliased as today

Santiago Pastorino (Apr 08 2019 at 21:42, on Zulip):

@eddyb @oli to start with before diving into zillions of changes

Santiago Pastorino (Apr 08 2019 at 21:42, on Zulip):

do we want this changes in the structures?

Santiago Pastorino (Apr 08 2019 at 21:42, on Zulip):

check out a couple of FIXME that are there

Santiago Pastorino (Apr 08 2019 at 21:43, on Zulip):

I think we don't want ProjectionElem and neither PlaceElem given that with the new structures there's no separate concept of an projection elem

Santiago Pastorino (Apr 08 2019 at 21:44, on Zulip):

there's base and elem altogether

Santiago Pastorino (Apr 08 2019 at 21:47, on Zulip):

I left them still, maybe is worth to make that change compile and then removing those structs and adapt them to use, instead of the ProjectionElem that has the FIXME we want to adapt the code and use the new Projection both are generic and instead of the PlaceElem that has the FIXME we want to adapt the code and use the new PlaceProjection both are concrete

Santiago Pastorino (Apr 08 2019 at 21:48, on Zulip):

tomorrow I will probably be able to finish this part, your pointers would be nice so I don't dive into a nightmare of changes that go in the wrong direction :)

oli (Apr 09 2019 at 06:05, on Zulip):

@Santiago Pastorino that change doesn't work until we actually get rid of the recursion

oli (Apr 09 2019 at 06:06, on Zulip):

you'll need to keep Projection and ProjectionElem around. Your new Projection variant is just direct recursion without any metadata.

oli (Apr 09 2019 at 06:09, on Zulip):

The base field could just be Option<Box<Projection<B, V, T>> and None for the leaf-situation

oli (Apr 09 2019 at 06:09, on Zulip):

then the ProjectionElem enum doesn't need any changes

oli (Apr 09 2019 at 06:09, on Zulip):

Sorry about misleading you there, I didn't think that through

oli (Apr 09 2019 at 06:10, on Zulip):

You're right, once we move to slices, the Projection type can be removed and we'll just have ProjectionElem, which can then be renamed to Projection

Santiago Pastorino (Apr 09 2019 at 15:01, on Zulip):

so ... I'm now trying to understand exactly what this step is about

Santiago Pastorino (Apr 09 2019 at 15:02, on Zulip):

@oli so you meant to leave Projection and ProjectionElem as they are currently in the code?

Santiago Pastorino (Apr 09 2019 at 15:02, on Zulip):

and just change Place from enum to struct?

Santiago Pastorino (Apr 09 2019 at 15:02, on Zulip):

/cc @eddyb

Santiago Pastorino (Apr 09 2019 at 15:03, on Zulip):

trying to understand the step from the doc that says ...

Santiago Pastorino (Apr 09 2019 at 15:03, on Zulip):
    struct Place {
        base: PlaceBase,
        projection: PlaceProjection,
    }
    enum PlaceProjection {
        Base,
        Projection(Box<PlaceProjection>),
        Deref,
        Index(..),
        ...

..

eddyb (Apr 09 2019 at 15:03, on Zulip):

I'm lacking a lot of context, sorry

eddyb (Apr 09 2019 at 15:03, on Zulip):

the Projection variant there is obviously wrong

eddyb (Apr 09 2019 at 15:03, on Zulip):

it's just an unary encoding

Santiago Pastorino (Apr 09 2019 at 15:04, on Zulip):

@eddyb I'm just talking about https://paper.dropbox.com/doc/Place-2.0-current-PR-status--Aa4SgFPQ34YAIfVjdc_oBL7DAg-vmbnFv8VkCEuL57QfWWMH that step that says change Place to

eddyb (Apr 09 2019 at 15:04, on Zulip):

Projection(Projection(Projection(Base))) is the number 3 with no other information :P

Santiago Pastorino (Apr 09 2019 at 15:04, on Zulip):

yes

Santiago Pastorino (Apr 09 2019 at 15:04, on Zulip):

that's wrong

eddyb (Apr 09 2019 at 15:04, on Zulip):

it should be a list

Santiago Pastorino (Apr 09 2019 at 15:04, on Zulip):

I'm more trying to understand what you wanted to do with the code more than what I'd do :)

eddyb (Apr 09 2019 at 15:04, on Zulip):

which means you just want a linked list an enum that's very much like today for the actual elem

Santiago Pastorino (Apr 09 2019 at 15:05, on Zulip):

I mean, you know way more than me about the compiler so my ideas may miss a lot of things

eddyb (Apr 09 2019 at 15:05, on Zulip):

me? I don't know, I didn't come up with this linked list form

Santiago Pastorino (Apr 09 2019 at 15:05, on Zulip):

when I've said you I was referring to the compiler team members that come with this idea

Santiago Pastorino (Apr 09 2019 at 15:05, on Zulip):

I thought those were you and @oli :)

Santiago Pastorino (Apr 09 2019 at 15:05, on Zulip):

but whatever

eddyb (Apr 09 2019 at 15:05, on Zulip):

I wasn't involved, sorry :(

Santiago Pastorino (Apr 09 2019 at 15:06, on Zulip):

no worries

Santiago Pastorino (Apr 09 2019 at 15:06, on Zulip):

/cc @oli

Santiago Pastorino (Apr 09 2019 at 15:11, on Zulip):

I guess this https://gist.github.com/spastorino/f5b88513e866cf7e0acf23cef8e507c9 is what you're talking about

eddyb (Apr 09 2019 at 15:12, on Zulip):

mmmmaybe?

Santiago Pastorino (Apr 09 2019 at 15:12, on Zulip):

hehe, yes

eddyb (Apr 09 2019 at 15:12, on Zulip):

projection: Option<Box<PlaceProjection<'tcx>>>,

eddyb (Apr 09 2019 at 15:12, on Zulip):

I think this would make it equivalent

Santiago Pastorino (Apr 09 2019 at 15:13, on Zulip):

to get rid of base in projection?

eddyb (Apr 09 2019 at 15:14, on Zulip):

no

eddyb (Apr 09 2019 at 15:14, on Zulip):

to allow no projections, like Place::Base today

Santiago Pastorino (Apr 09 2019 at 15:25, on Zulip):

@eddyb yeah, true added that but you still need the option in base

eddyb (Apr 09 2019 at 15:26, on Zulip):

yeah you need both

Santiago Pastorino (Apr 10 2019 at 02:46, on Zulip):

@oli please when you are around confirm what do we want to do exactly :)

Santiago Pastorino (Apr 10 2019 at 02:46, on Zulip):

I'm a bit confused

Santiago Pastorino (Apr 10 2019 at 02:46, on Zulip):

my guess is that we want something like ...

Santiago Pastorino (Apr 10 2019 at 02:46, on Zulip):
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
index 8424c096e88..2120e741452 100644
--- a/src/librustc/mir/mod.rs
+++ b/src/librustc/mir/mod.rs
@@ -1900,11 +1900,11 @@ impl<'tcx> Debug for Statement<'tcx> {
 /// A path to a value; something that can be evaluated without
 /// changing or disturbing program state.
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable, HashStable)]
-pub enum Place<'tcx> {
-    Base(PlaceBase<'tcx>),
+pub struct Place<'tcx> {
+    base: PlaceBase<'tcx>,

     /// projection out of a place (access a field, deref a pointer, etc)
-    Projection(Box<PlaceProjection<'tcx>>),
+    projection: Option<Box<PlaceProjection<'tcx>>>,
 }

 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable, HashStable)]
@@ -1941,7 +1941,7 @@ impl_stable_hash_for!(struct Static<'tcx> {
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord,
          Hash, RustcEncodable, RustcDecodable, HashStable)]
 pub struct Projection<B, V, T> {
-    pub base: B,
+    pub next: Option<B>,
     pub elem: ProjectionElem<V, T>,
 }
Santiago Pastorino (Apr 10 2019 at 02:47, on Zulip):

the current Place is a thing that goes backwards

Santiago Pastorino (Apr 10 2019 at 02:47, on Zulip):

a.b.c is represented as .c -> .b -> .a

Santiago Pastorino (Apr 10 2019 at 02:47, on Zulip):

if we now have a base and projection at the same time I guess we have

Santiago Pastorino (Apr 10 2019 at 02:48, on Zulip):

a as the base

Santiago Pastorino (Apr 10 2019 at 02:48, on Zulip):

and in the projection we want .b to be the elem and the next to be the .c part, is that right?

Santiago Pastorino (Apr 10 2019 at 02:50, on Zulip):

if we want the new thing in this particular step to be backwards I don't know what represents to have first both base and projection, projection would be .c and base a? what would be that useful for?

Santiago Pastorino (Apr 10 2019 at 02:51, on Zulip):

what needs to be done is easy but I'm kind of stucked trying to understand what do we want to accomplish :/

Santiago Pastorino (Apr 10 2019 at 03:48, on Zulip):

btw, this is what I meant https://gist.github.com/spastorino/f5b88513e866cf7e0acf23cef8e507c9 with the from left to right idea

Santiago Pastorino (Apr 10 2019 at 03:51, on Zulip):

or it could even be a list of projections and stop that silly recursion

Santiago Pastorino (Apr 10 2019 at 03:52, on Zulip):

but I guess @oli had reasons to make a step that somehow resembles the current code

oli (Apr 10 2019 at 09:06, on Zulip):

@Santiago Pastorino I was thinking that we'd end up with (a, .c -> .b) as a first step

oli (Apr 10 2019 at 09:07, on Zulip):

having an intermediate step where the order is (a, .b -> .c) is unnecessary I think, at that point we can go to the slice directly

oli (Apr 10 2019 at 09:07, on Zulip):

so essentially what you had in https://gist.github.com/spastorino/f5b88513e866cf7e0acf23cef8e507c9

Santiago Pastorino (Apr 10 2019 at 11:11, on Zulip):

@oli I guess you meant this https://gist.github.com/spastorino/f5b88513e866cf7e0acf23cef8e507c9/ce2b1c8d7017d49938921d5ea2dcccac899c7fe7 specific revision

oli (Apr 10 2019 at 11:11, on Zulip):

yes

Santiago Pastorino (Apr 10 2019 at 11:11, on Zulip):

the weird thing there is the option in base's Projection field

Santiago Pastorino (Apr 10 2019 at 11:12, on Zulip):

so ... that's None in the .b case?

Santiago Pastorino (Apr 10 2019 at 11:15, on Zulip):

I meant, if we want this (a, .c -> .b), .c base would be .b and .b base would be None

Santiago Pastorino (Apr 10 2019 at 11:15, on Zulip):

which is a bit weird

Santiago Pastorino (Apr 10 2019 at 11:16, on Zulip):

that in the case base is a Projection

Santiago Pastorino (Apr 10 2019 at 11:19, on Zulip):

@oli but you yeah, you could handle that considering that when the base of a Projection is None you just go to the original Place, a in this case

oli (Apr 10 2019 at 11:19, on Zulip):

yea, it's weird because we need two Options

Santiago Pastorino (Apr 10 2019 at 11:19, on Zulip):

ok

Santiago Pastorino (Apr 10 2019 at 11:20, on Zulip):

my main question now is ... what are you foreseen exactly by using this structured in this way?

oli (Apr 10 2019 at 11:20, on Zulip):

I honestly did not see this when I was thinking about this transformation originally

Santiago Pastorino (Apr 10 2019 at 11:20, on Zulip):

instead of going just straight to use a Vec

oli (Apr 10 2019 at 11:20, on Zulip):

the important part of this transformation is the base field on Place

Santiago Pastorino (Apr 10 2019 at 11:21, on Zulip):

ok

oli (Apr 10 2019 at 11:21, on Zulip):

many algorithms just want that and don't care about the projections

Santiago Pastorino (Apr 10 2019 at 11:21, on Zulip):

you just want to transform to use a base and don't care about the rest

Santiago Pastorino (Apr 10 2019 at 11:21, on Zulip):

that's more or less the idea

oli (Apr 10 2019 at 11:21, on Zulip):

If we do the base field change and the slice change in two PRs, then we have fewer places to modify at once

oli (Apr 10 2019 at 11:21, on Zulip):

yes, exactly

Santiago Pastorino (Apr 10 2019 at 11:21, on Zulip):

perfect

Santiago Pastorino (Apr 10 2019 at 11:22, on Zulip):

I honestly did not see this when I was thinking about this transformation originally

you did not see what? sorry

oli (Apr 10 2019 at 11:22, on Zulip):

that you'd end up needing options and whatnot

Santiago Pastorino (Apr 10 2019 at 11:22, on Zulip):

the fact that we needed Option in ... :)

Santiago Pastorino (Apr 10 2019 at 11:22, on Zulip):

ok, thanks for the info

Santiago Pastorino (Apr 10 2019 at 11:22, on Zulip):

need to leave for a bit and a bit later going to do this

Santiago Pastorino (Apr 10 2019 at 11:23, on Zulip):

but may be back and start to read your proposal to see what's coming next and may ask questions

Santiago Pastorino (Apr 10 2019 at 11:23, on Zulip):

so I have a better picture of the rest of the work

Santiago Pastorino (Apr 10 2019 at 14:28, on Zulip):

@oli the structure we come up with is wrong :)

Santiago Pastorino (Apr 10 2019 at 14:29, on Zulip):

at least I think it is

Santiago Pastorino (Apr 10 2019 at 14:29, on Zulip):

for instance

Santiago Pastorino (Apr 10 2019 at 14:32, on Zulip):

sorry got distracted

Santiago Pastorino (Apr 10 2019 at 14:32, on Zulip):
    /// Finds the innermost `Local` from this `Place`, *if* it is either a local itself or
    /// a single deref of a local.
    //
    // FIXME: can we safely swap the semantics of `fn base_local` below in here instead?
    pub fn local(&self) -> Option<Local> {
        match self {
            Place::Base(PlaceBase::Local(local)) |
            Place::Projection(box Projection {
                base: Place::Base(PlaceBase::Local(local)),
                elem: ProjectionElem::Deref,
            }) => Some(*local),
            _ => None,
        }
    }
Santiago Pastorino (Apr 10 2019 at 14:32, on Zulip):

@oli in order to implement this exactly what do you do?

oli (Apr 10 2019 at 14:33, on Zulip):

you match on self.base I presume (Self is Place, right?)

Santiago Pastorino (Apr 10 2019 at 14:33, on Zulip):

yes

oli (Apr 10 2019 at 14:33, on Zulip):

irgnore all projections

oli (Apr 10 2019 at 14:33, on Zulip):

oh

oli (Apr 10 2019 at 14:33, on Zulip):

I misread

Santiago Pastorino (Apr 10 2019 at 14:33, on Zulip):

but well, the code is saying ... :)

oli (Apr 10 2019 at 14:33, on Zulip):

yes

Santiago Pastorino (Apr 10 2019 at 14:34, on Zulip):

I meant, we need a way in projections a way to go to the base place as a PlaceBase

oli (Apr 10 2019 at 14:34, on Zulip):

so, checking for a local is the first step, and then check that the projections are either empty, or contain just a deref projection

oli (Apr 10 2019 at 14:34, on Zulip):

the projections don't need to know about the base

Santiago Pastorino (Apr 10 2019 at 14:35, on Zulip):

hmmm in the way I was reading the idea was ...

oli (Apr 10 2019 at 14:35, on Zulip):

if you want you can also do match (self.base, self.projection) to get back almost the same code that shows up here, now

Santiago Pastorino (Apr 10 2019 at 14:36, on Zulip):

but according to what I've said earlier (in case I was right) self.base is not really the base of the self.projection element

Santiago Pastorino (Apr 10 2019 at 14:36, on Zulip):

is the top base

Santiago Pastorino (Apr 10 2019 at 14:36, on Zulip):

so

Santiago Pastorino (Apr 10 2019 at 14:36, on Zulip):

a.b.c

oli (Apr 10 2019 at 14:36, on Zulip):

the first arm would be (PlaceBase::Local(local), None) | (PlaceBase::Local(local), Some(box Projection { base: None, elem: ProjectionElem::Deref }))

Santiago Pastorino (Apr 10 2019 at 14:36, on Zulip):

I have c in the projection and a in base

Santiago Pastorino (Apr 10 2019 at 14:37, on Zulip):

the old code was c in the projection and b in base

Santiago Pastorino (Apr 10 2019 at 14:37, on Zulip):

so it was checking that c was a Deref and b was a Local

Santiago Pastorino (Apr 10 2019 at 14:37, on Zulip):

I don't have any way to do that now

oli (Apr 10 2019 at 14:37, on Zulip):

right, but all this function cares about is having either a local or a deref projeciton on a local

Santiago Pastorino (Apr 10 2019 at 14:38, on Zulip):

right a.b.c is not a Deref

Santiago Pastorino (Apr 10 2019 at 14:38, on Zulip):

you're right

Santiago Pastorino (Apr 10 2019 at 14:38, on Zulip):

thanks

oli (Apr 10 2019 at 14:39, on Zulip):

we either have local or local.deref

oli (Apr 10 2019 at 14:39, on Zulip):

all other cases are irrelevant for this function

Santiago Pastorino (Apr 10 2019 at 14:39, on Zulip):

yes yes

Santiago Pastorino (Apr 10 2019 at 14:39, on Zulip):

I was more thinking about fields

oli (Apr 10 2019 at 14:39, on Zulip):

I still have some notes from the all hands that we'll want to nuke Deref at some point anyway. Your changes will make that so much easier!

oli (Apr 10 2019 at 14:39, on Zulip):

then this function becomes nonexistant

Santiago Pastorino (Apr 10 2019 at 14:40, on Zulip):

:)

Santiago Pastorino (Apr 10 2019 at 14:40, on Zulip):

ok ok, gonna keep this going

Santiago Pastorino (Apr 10 2019 at 14:40, on Zulip):

after a couple of changes I may push something for you to check just in case everything feels in the right direction

oli (Apr 10 2019 at 14:40, on Zulip):

I can have a look immediately if you want

Santiago Pastorino (Apr 10 2019 at 14:41, on Zulip):

there are so many changes to do that I don't want to end with 2.000 lines changes but have done the wrong thing :')

Santiago Pastorino (Apr 10 2019 at 14:41, on Zulip):

I can have a look immediately if you want

cool, gonna try to push something in a couple of minutes

Santiago Pastorino (Apr 10 2019 at 14:41, on Zulip):

is going to lack a lot of things but at least fix this file and see if everything looks more or less ok

Santiago Pastorino (Apr 10 2019 at 15:31, on Zulip):

@oli I've figured that you probably want to check this https://github.com/spastorino/rust/commit/e8b4b773e421b8c1feb01c59817a2962f5ab3d45

oli (Apr 10 2019 at 15:36, on Zulip):

@Santiago Pastorino mostly some nits on the pattern matches which can be simpler now

oli (Apr 10 2019 at 15:37, on Zulip):

the changes in the datastructure lgtm

Santiago Pastorino (Apr 10 2019 at 15:57, on Zulip):

@oli your comments are right

Santiago Pastorino (Apr 10 2019 at 15:58, on Zulip):

(deleted)

Santiago Pastorino (Apr 10 2019 at 23:36, on Zulip):

@oli ...

Santiago Pastorino (Apr 10 2019 at 23:37, on Zulip):
    pub fn is_upvar_field_projection<'cx, 'gcx>(&self, mir: &'cx Mir<'tcx>,
                                                tcx: &TyCtxt<'cx, 'gcx, 'tcx>) -> Option<Field> {
        let (place, by_ref) = if let Place::Projection(ref proj) = self {
            if let ProjectionElem::Deref = proj.elem {
                (&proj.base, true)
            } else {
                (self, false)
            }
        } else {
            (self, false)
        };

        match place {
            Place::Projection(ref proj) => match proj.elem {
                ProjectionElem::Field(field, _ty) => {
                    let base_ty = proj.base.ty(mir, *tcx).ty;

                    if (base_ty.is_closure() || base_ty.is_generator()) &&
                        (!by_ref || mir.upvar_decls[field.index()].by_ref)
                    {
                        Some(field)
                    } else {
                        None
                    }
                },
                _ => None,
            }
            _ => None,
        }
    }
Santiago Pastorino (Apr 10 2019 at 23:37, on Zulip):

this suggests that what we talked earlier is not ok?

Santiago Pastorino (Apr 10 2019 at 23:37, on Zulip):

I mean, there place can be ... first a Deref and then a Field

Santiago Pastorino (Apr 10 2019 at 23:37, on Zulip):

otherwise the code doesn't make a lot of sense

Santiago Pastorino (Apr 10 2019 at 23:38, on Zulip):

wonder a couple of things

Santiago Pastorino (Apr 10 2019 at 23:38, on Zulip):

first what to do there

Santiago Pastorino (Apr 10 2019 at 23:39, on Zulip):

and second what are the possible combination of Places, unsure if I'm properly understanding all the mixtures that can happen

Santiago Pastorino (Apr 10 2019 at 23:42, on Zulip):

my guess is that what we talked earlier is fine but unsure how this code is possible

Santiago Pastorino (Apr 10 2019 at 23:43, on Zulip):

I mean, how you can have a Place that is a projection deref, and it's base is a Field

Santiago Pastorino (Apr 10 2019 at 23:44, on Zulip):

a.b and b being Box?

oli (Apr 11 2019 at 07:08, on Zulip):

@Santiago Pastorino basically any code that matched on place before, should now match on place.projection

oli (Apr 11 2019 at 07:08, on Zulip):

you should not need to change anything structurally

oli (Apr 11 2019 at 07:08, on Zulip):

where you used to match on place and checked for a Place::Base variant, you don't need to match but can access place.base

Santiago Pastorino (Apr 11 2019 at 11:07, on Zulip):

@oli I'm a bit confused

Santiago Pastorino (Apr 11 2019 at 11:08, on Zulip):

https://github.com/spastorino/rust/commit/e8b4b773e421b8c1feb01c59817a2962f5ab3d45#diff-f0577ac900ffbd36d3bb3421a928cbbdL2043

Santiago Pastorino (Apr 11 2019 at 11:08, on Zulip):

maybe asking a more general question than the code there

Santiago Pastorino (Apr 11 2019 at 11:09, on Zulip):

I'm not sure about all the possibilities you can have on all place

Santiago Pastorino (Apr 11 2019 at 11:09, on Zulip):

for instance, is possible to have something like a -> b -> c and c being a projection which is a Deref and b being a Field?

Santiago Pastorino (Apr 11 2019 at 11:09, on Zulip):

or something like that

oli (Apr 11 2019 at 11:09, on Zulip):

yes

Santiago Pastorino (Apr 11 2019 at 11:10, on Zulip):

what code produces that?

Santiago Pastorino (Apr 11 2019 at 11:10, on Zulip):

if c is box?

Santiago Pastorino (Apr 11 2019 at 11:11, on Zulip):

I'm asking this because from yesterday I thought you were saying that that's not possible

Santiago Pastorino (Apr 11 2019 at 11:11, on Zulip):

in particular https://github.com/spastorino/rust/commit/e8b4b773e421b8c1feb01c59817a2962f5ab3d45#diff-f0577ac900ffbd36d3bb3421a928cbbdL2043 this code is not equivalent

oli (Apr 11 2019 at 11:12, on Zulip):

that would be Place { base: a, projection: Some(Box::new(PlaceProjection { projection ProjectionKind::Deref, base: Some(Box::new(PlaceProjection { base: None, Projection: ProjectionKind::Field } (plus some closing brackets

Santiago Pastorino (Apr 11 2019 at 11:12, on Zulip):

yes, but I meant, what user code produces that structure?

oli (Apr 11 2019 at 11:13, on Zulip):

*a.b I think

oli (Apr 11 2019 at 11:13, on Zulip):

so the AST is Deref(Field(a))

Santiago Pastorino (Apr 11 2019 at 11:13, on Zulip):

ok

Santiago Pastorino (Apr 11 2019 at 11:13, on Zulip):

so as I was saying ...

Santiago Pastorino (Apr 11 2019 at 11:13, on Zulip):

in particular https://github.com/spastorino/rust/commit/e8b4b773e421b8c1feb01c59817a2962f5ab3d45#diff-f0577ac900ffbd36d3bb3421a928cbbdL2043 this code is not equivalent

this ^^^ seems wrong

oli (Apr 11 2019 at 11:14, on Zulip):

what part about it? It looks like the correct transformation from master to your datastructures

Santiago Pastorino (Apr 11 2019 at 11:15, on Zulip):

so if you have something like a -> b -> c

Santiago Pastorino (Apr 11 2019 at 11:15, on Zulip):

c is the projection which is a Field

Santiago Pastorino (Apr 11 2019 at 11:15, on Zulip):

b is a Deref

Santiago Pastorino (Apr 11 2019 at 11:16, on Zulip):

and a is something else

Santiago Pastorino (Apr 11 2019 at 11:16, on Zulip):

in the old code c is the deref and it's base is b

Santiago Pastorino (Apr 11 2019 at 11:16, on Zulip):

now when I use base I'm going straight to the top

Santiago Pastorino (Apr 11 2019 at 11:16, on Zulip):

which is a

Santiago Pastorino (Apr 11 2019 at 11:16, on Zulip):

so I'm skipping b entirely

Santiago Pastorino (Apr 11 2019 at 11:18, on Zulip):

sorry my example is bad

Santiago Pastorino (Apr 11 2019 at 11:18, on Zulip):

let me fix that

oli (Apr 11 2019 at 11:18, on Zulip):

but you ensured there's no b via the None

oli (Apr 11 2019 at 11:18, on Zulip):

https://github.com/spastorino/rust/commit/e8b4b773e421b8c1feb01c59817a2962f5ab3d45#diff-f0577ac900ffbd36d3bb3421a928cbbdR2057

Santiago Pastorino (Apr 11 2019 at 11:19, on Zulip):

yes, the code seems correct

Santiago Pastorino (Apr 11 2019 at 11:19, on Zulip):

I was confused by something else in this example

Santiago Pastorino (Apr 11 2019 at 11:19, on Zulip):

because Local can't be in the middle

Santiago Pastorino (Apr 11 2019 at 11:19, on Zulip):

I meant, is not a projection

Santiago Pastorino (Apr 11 2019 at 11:19, on Zulip):

so the None plus using a base Local is enough in this case

Santiago Pastorino (Apr 11 2019 at 11:21, on Zulip):

Santiago Pastorino basically any code that matched on place before, should now match on place.projection

anyway, I don't think this is that simple

Santiago Pastorino (Apr 11 2019 at 11:21, on Zulip):

:P

oli (Apr 11 2019 at 11:21, on Zulip):

you can't ever have a local in the middle

Santiago Pastorino (Apr 11 2019 at 11:21, on Zulip):

yes

Santiago Pastorino (Apr 11 2019 at 11:21, on Zulip):

I mean, you can't :)

oli (Apr 11 2019 at 11:21, on Zulip):

/me is confused

Santiago Pastorino (Apr 11 2019 at 11:22, on Zulip):

sorry, I meant you can't have a local in the middle

oli (Apr 11 2019 at 11:22, on Zulip):

I'm very sure it is as simple as just matching on place.projection

oli (Apr 11 2019 at 11:22, on Zulip):

oh I got that

Santiago Pastorino (Apr 11 2019 at 11:22, on Zulip):

I was agreeing with you

oli (Apr 11 2019 at 11:22, on Zulip):

I'm wondering whether you have encountered situations where you can't just match on place.projection

Santiago Pastorino (Apr 11 2019 at 11:22, on Zulip):

I was thinking about Deref and Field but saw that the example I was pointing at you was about Local

Santiago Pastorino (Apr 11 2019 at 11:23, on Zulip):
    pub fn is_upvar_field_projection<'cx, 'gcx>(&self, mir: &'cx Mir<'tcx>,
                                                tcx: &TyCtxt<'cx, 'gcx, 'tcx>) -> Option<Field> {
        let (place, by_ref) = if let Place::Projection(ref proj) = self {
            if let ProjectionElem::Deref = proj.elem {
                (&proj.base, true)
            } else {
                (self, false)
            }
        } else {
            (self, false)
        };

        match place {
            Place::Projection(ref proj) => match proj.elem {
                ProjectionElem::Field(field, _ty) => {
                    let base_ty = proj.base.ty(mir, *tcx).ty;

                    if (base_ty.is_closure() || base_ty.is_generator()) &&
                        (!by_ref || mir.upvar_decls[field.index()].by_ref)
                    {
                        Some(field)
                    } else {
                        None
                    }
                },
                _ => None,
            }
            _ => None,
        }
    }

for instance, in this case ^^^

Santiago Pastorino (Apr 11 2019 at 11:23, on Zulip):

see the first place binding

Santiago Pastorino (Apr 11 2019 at 11:23, on Zulip):

that's = to self or self.base

Santiago Pastorino (Apr 11 2019 at 11:24, on Zulip):

those with the new code are a Place or a PlaceBase

Santiago Pastorino (Apr 11 2019 at 11:24, on Zulip):

different types

Santiago Pastorino (Apr 11 2019 at 11:24, on Zulip):

I meant, is not that hard to fix that but it's not just matching in self.projection :P

Santiago Pastorino (Apr 11 2019 at 11:24, on Zulip):

that's what I meant

oli (Apr 11 2019 at 11:29, on Zulip):

oh, that still works

oli (Apr 11 2019 at 11:30, on Zulip):

you match on place.projection and either return place.projection or proj.base

oli (Apr 11 2019 at 11:30, on Zulip):

All this code does is eat one Deref if there was one

oli (Apr 11 2019 at 11:31, on Zulip):

In fact this is even easier, since the outer if let can be removed and you can just use let proj = self.projection?;

oli (Apr 11 2019 at 11:32, on Zulip):

which will save you the redundant match place below.

oli (Apr 11 2019 at 11:32, on Zulip):

You should probably rename the place variable to proj though

oli (Apr 11 2019 at 11:33, on Zulip):

I sincerely hope my ramblings make any sense at all

oli (Apr 11 2019 at 11:33, on Zulip):

basically this entire function does not care about self.base

Santiago Pastorino (Apr 11 2019 at 12:24, on Zulip):

hey back

Santiago Pastorino (Apr 11 2019 at 12:24, on Zulip):

ohh yes, your comments make a lot of sense

Santiago Pastorino (Apr 11 2019 at 12:24, on Zulip):

thanks :)

Santiago Pastorino (Apr 11 2019 at 14:25, on Zulip):
    pub fn is_upvar_field_projection<'cx, 'gcx>(&self, mir: &'cx Mir<'tcx>,
                                                tcx: &TyCtxt<'cx, 'gcx, 'tcx>) -> Option<Field> {
        let (opt_proj, by_ref) = if let Some(proj) = self.projection {
            if let ProjectionElem::Deref = proj.elem {
                (proj.base, true)
            } else {
                (Some(proj), false)
            }
        } else {
            (None, false)
        };

        match opt_proj {
            Some(ref proj) => match proj.elem {
                ProjectionElem::Field(field, _ty) => {
                    let base_ty = if let Some(box base) = proj.base {
                        let mut place_ty = self.base.ty(mir, *tcx);
                        place_ty.projection_ty(*tcx, &base.elem).ty
                    } else {
                        self.base.ty(mir, *tcx).ty
                    };

                    if (base_ty.is_closure() || base_ty.is_generator()) &&
                        (!by_ref || mir.upvar_decls[field.index()].by_ref)
                    {
                        Some(field)
                    } else {
                        None
                    }
                },
                _ => None,
            }
            None => None,
        }
    }
Santiago Pastorino (Apr 11 2019 at 14:25, on Zulip):

@oli back to this thing :)

Santiago Pastorino (Apr 11 2019 at 14:25, on Zulip):

this would be the straightforward way to translate that method

Santiago Pastorino (Apr 11 2019 at 14:25, on Zulip):

which sucks a lot

Santiago Pastorino (Apr 11 2019 at 14:25, on Zulip):

well actually the original method had a lot to improve

Santiago Pastorino (Apr 11 2019 at 14:26, on Zulip):

but I guess it's not wise to refactor methods meanwhile doing this

oli (Apr 11 2019 at 14:29, on Zulip):

that's not too bad

Santiago Pastorino (Apr 11 2019 at 14:29, on Zulip):

a bad part about the new thing is that is not that direct to get the ty of a PlaceProjection self.base was before a Place so we could just call ty

oli (Apr 11 2019 at 14:29, on Zulip):

add a FIXME about cleaning it up?

Santiago Pastorino (Apr 11 2019 at 14:30, on Zulip):

now we need to do the dance I've done

Santiago Pastorino (Apr 11 2019 at 14:30, on Zulip):

yes

Santiago Pastorino (Apr 11 2019 at 14:30, on Zulip):

let me push again what I have

Santiago Pastorino (Apr 11 2019 at 14:30, on Zulip):

I'm going to be pushing WIP 1/2/3 etc so it's easier for you to review

oli (Apr 11 2019 at 14:30, on Zulip):

oh, that type extraction... hmm

Santiago Pastorino (Apr 11 2019 at 14:31, on Zulip):

the thing I'm going to push has amended already what I've pushed yesterday

Santiago Pastorino (Apr 11 2019 at 14:31, on Zulip):

I've just realized that would be easier if I keep pushing things and at the end I do a huge squash

Santiago Pastorino (Apr 11 2019 at 14:31, on Zulip):

just to simplify you following up the discussions and changes

Santiago Pastorino (Apr 11 2019 at 14:31, on Zulip):

oh, that type extraction... hmm

I thought about adding a ty method to PlaceProjection

Santiago Pastorino (Apr 11 2019 at 14:32, on Zulip):

the problem is that I need PlaceBase :)

Santiago Pastorino (Apr 11 2019 at 14:32, on Zulip):

or maybe better to add a base_ty to Place

Santiago Pastorino (Apr 11 2019 at 14:32, on Zulip):

or super_ty

Santiago Pastorino (Apr 11 2019 at 14:33, on Zulip):

which goes one step up and if there's a base projection it uses that otherwise goes to PlaceBase

Santiago Pastorino (Apr 11 2019 at 14:33, on Zulip):

I'm not liking that much to talk about base in both cases

oli (Apr 11 2019 at 14:34, on Zulip):

yea, it's weird

Santiago Pastorino (Apr 11 2019 at 14:34, on Zulip):

there's PlaceBase root base and there's the base field in PlaceProjection

Santiago Pastorino (Apr 11 2019 at 14:34, on Zulip):

yea, it's weird

what's weird from all that I've said? :P, everything :P

oli (Apr 11 2019 at 14:35, on Zulip):

the one in the projection is less base and more outer_projection or sth

Santiago Pastorino (Apr 11 2019 at 14:35, on Zulip):

yes

Santiago Pastorino (Apr 11 2019 at 14:36, on Zulip):

I guess I can add a rename this fixme

Santiago Pastorino (Apr 11 2019 at 14:36, on Zulip):

I think doing that now will cause some pain?

Santiago Pastorino (Apr 11 2019 at 14:36, on Zulip):

hmmm do you think it worth?

Santiago Pastorino (Apr 11 2019 at 14:36, on Zulip):

maybe it does to do all now

oli (Apr 11 2019 at 14:39, on Zulip):

yea, either do it now or never

Santiago Pastorino (Apr 11 2019 at 14:39, on Zulip):

ok, gonna change that

oli (Apr 11 2019 at 14:39, on Zulip):

that field won't exist for long

Santiago Pastorino (Apr 11 2019 at 14:39, on Zulip):

ahh right, so it doesn't worth then

Santiago Pastorino (Apr 11 2019 at 14:40, on Zulip):

or maybe better to add a base_ty to Place

does this worth?

Santiago Pastorino (Apr 11 2019 at 14:40, on Zulip):

it's not technically base

Santiago Pastorino (Apr 11 2019 at 14:40, on Zulip):
-                    let base_ty = proj.base.ty(mir, *tcx).ty;
+                    let base_ty = if let Some(box base) = proj.base {
+                        let mut place_ty = self.base.ty(mir, *tcx);
+                        place_ty.projection_ty(*tcx, &base.elem).ty
+                    } else {
+                        self.base.ty(mir, *tcx).ty
+                    };
Santiago Pastorino (Apr 11 2019 at 14:40, on Zulip):

it's a method that solves that if stuff

Santiago Pastorino (Apr 11 2019 at 14:41, on Zulip):

basically getting the ty of one level up

Santiago Pastorino (Apr 11 2019 at 14:41, on Zulip):

so outer_projection or base

Santiago Pastorino (Apr 11 2019 at 14:44, on Zulip):

@oli that's what it is for now https://github.com/spastorino/rust/commit/38e66f7397599d48a78aebcf23e2d4fb3485a9a3

oli (Apr 11 2019 at 14:47, on Zulip):

hmm... what if you made it let mut base_ty = self.base.ty(mir, *tcx); if let Some(base) = &proj.base { base_ty = base_ty.projection_ty(*tcx, &base.elem); }?

oli (Apr 11 2019 at 14:47, on Zulip):

not sure what the return type of projection_ty is

Santiago Pastorino (Apr 11 2019 at 14:58, on Zulip):

yeah that works

Santiago Pastorino (Apr 11 2019 at 15:01, on Zulip):

oli that's what it is for now https://github.com/spastorino/rust/commit/38e66f7397599d48a78aebcf23e2d4fb3485a9a3

on top of this for you to review, I've pushed that change here https://github.com/spastorino/rust/commit/fc3faa5d7def51227fae05ae43e1de0e397a05c1

Santiago Pastorino (Apr 11 2019 at 16:28, on Zulip):

@oli another similar thing ...

Santiago Pastorino (Apr 11 2019 at 16:29, on Zulip):
            fn super_projection(&mut self,
                                proj: & $($mutability)? PlaceProjection<'tcx>,
                                context: PlaceContext<'tcx>,
                                location: Location) {
                let Projection { base, elem } = proj;
                let context = if context.is_mutating_use() {
                    PlaceContext::MutatingUse(MutatingUseContext::Projection)
                } else {
                    PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection)
                };
                self.visit_place(base, context, location);
                self.visit_projection_elem(elem, location);
            }
Santiago Pastorino (Apr 11 2019 at 16:29, on Zulip):

visit_place takes base which is not a Place anymore, right now it's a PlaceProjection

Santiago Pastorino (Apr 11 2019 at 16:30, on Zulip):

I'm not sure how did you envision to play with this stuff

Santiago Pastorino (Apr 11 2019 at 16:30, on Zulip):

should I just have visit_place_projection thing?

Santiago Pastorino (Apr 11 2019 at 16:32, on Zulip):

well there's already one, going to play with this for a bit to see if things end in an equivalent way :)

oli (Apr 11 2019 at 18:26, on Zulip):

depends on the users...

oli (Apr 11 2019 at 18:26, on Zulip):

can we get away with not calling visit_place here?

oli (Apr 11 2019 at 18:27, on Zulip):

if the users don't care about order, just completeness, we can easily reshuffle this

oli (Apr 11 2019 at 18:28, on Zulip):

basically replace the self.visit_place call with if let Some(proj) = proj.base { self.visit_projection(proj, context, location); }

oli (Apr 11 2019 at 18:29, on Zulip):

The only situation where we'd call visit_place would be wherever the visitor is processing the actual Place type and then we call it on the PlaceBase there

Santiago Pastorino (Apr 11 2019 at 18:36, on Zulip):

@oli in that particular place I've added a visit_place_base

Santiago Pastorino (Apr 11 2019 at 18:37, on Zulip):

made visit_place adapt to that

oli (Apr 11 2019 at 18:37, on Zulip):

ah, that makes sense

Santiago Pastorino (Apr 11 2019 at 18:37, on Zulip):

maybe better if I push this :)

Santiago Pastorino (Apr 11 2019 at 18:37, on Zulip):

ok

Santiago Pastorino (Apr 11 2019 at 18:37, on Zulip):
@@ -725,19 +732,27 @@ macro_rules! make_mir_visitor {
                             place: & $($mutability)? Place<'tcx>,
                             context: PlaceContext<'tcx>,
                             location: Location) {
+                let Place { base, projection } = place;
+
+                self.visit_place_base(base, context, location);
+
+                if let Some(proj) = projection {
+                    self.visit_projection(proj, context, location);
+                }
+            }
Santiago Pastorino (Apr 11 2019 at 18:37, on Zulip):

that's visit_place

Santiago Pastorino (Apr 11 2019 at 18:38, on Zulip):

well super_place, but you got what I meant :P

Santiago Pastorino (Apr 11 2019 at 18:38, on Zulip):

the default implementation of visit_place

Santiago Pastorino (Apr 11 2019 at 18:38, on Zulip):

found weird that, that's called super :)

oli (Apr 11 2019 at 18:50, on Zulip):

heh, half the compiler uses super_* the other half uses walk_*

Santiago Pastorino (Apr 11 2019 at 19:23, on Zulip):

:)

Santiago Pastorino (Apr 11 2019 at 19:23, on Zulip):

@oli as soon as you have time to review something let me know and I can push

Santiago Pastorino (Apr 11 2019 at 19:24, on Zulip):

there are already two WIP X commits up and I have a bunch of things in locally

Santiago Pastorino (Apr 11 2019 at 19:25, on Zulip):

btw ...

Santiago Pastorino (Apr 11 2019 at 19:25, on Zulip):
         &mut self,
         mir_place: &mir::Place<'tcx>
     ) -> EvalResult<'tcx, PlaceTy<'tcx, M::PointerTag>> {
-        use rustc::mir::Place::*;
         use rustc::mir::PlaceBase;
         let place = match *mir_place {
-            Base(PlaceBase::Local(mir::RETURN_PLACE)) => match self.frame().return_place {
+            Place {
+                base: PlaceBase::Local(mir::RETURN_PLACE),
+                projection: None,
+            } => match self.frame().return_place {
                 Some(return_place) =>
                     // We use our layout to verify our assumption; caller will validate
                     // their layout on return.
@@ -638,7 +645,10 @@ where
                     },
                 None => return err!(InvalidNullPointerUsage),
             },
-            Base(PlaceBase::Local(local)) => PlaceTy {
+            Place {
+                base: PlaceBase::Local(local),
+                projection: None,
+            } => PlaceTy {
                 place: Place::Local {
                     frame: self.cur_frame(),
                     local,
@@ -646,7 +656,10 @@ where
                 layout: self.layout_of_local(self.frame(), local, None)?,
             },

-            Projection(ref proj) => {
+            Place {
+                base: _,
+                projection: Some(ref proj),
+            } => {
                 let place = self.eval_place(&proj.base)?;
                 self.place_projection(place, &proj.elem)?
             }
Santiago Pastorino (Apr 11 2019 at 19:25, on Zulip):

I hate this kind of changes :P

Santiago Pastorino (Apr 11 2019 at 19:25, on Zulip):

I could match on place.base and place.projection and make some things simpler but some other things worser :/

Santiago Pastorino (Apr 11 2019 at 19:27, on Zulip):

I guess if I were trying to write better code on this design I'd split everything into something_place_base and something_projection methods but I don't think that worth given that this is going to be also changed ... so ... I guess the kind of diff I've pasted above is ok

Santiago Pastorino (Apr 11 2019 at 19:27, on Zulip):

anyway, let me know if it's not :)

oli (Apr 11 2019 at 20:00, on Zulip):

yea, lgtm

oli (Apr 11 2019 at 20:01, on Zulip):

I won't be able to review a lot until monday

oli (Apr 11 2019 at 20:01, on Zulip):

I'll try to find some time though, will be async most likely, so feel free to commit stuff and link me to it

Santiago Pastorino (Apr 11 2019 at 20:14, on Zulip):

@oli :+1:

Santiago Pastorino (Apr 11 2019 at 20:14, on Zulip):

unsure if you've already seen but ...

Santiago Pastorino (Apr 11 2019 at 20:15, on Zulip):

1. https://github.com/spastorino/rust/commit/38e66f7397599d48a78aebcf23e2d4fb3485a9a3
2. https://github.com/spastorino/rust/commit/fc3faa5d7def51227fae05ae43e1de0e397a05c1
3. https://github.com/spastorino/rust/commit/d308b77860c6fced35cf43b7417b12648e503271

Santiago Pastorino (Apr 11 2019 at 20:16, on Zulip):

the first two are things that I've shared already, 3 just pushed

Santiago Pastorino (Apr 11 2019 at 20:16, on Zulip):

anyway, now on I think most changes are mechanical

Santiago Pastorino (Apr 11 2019 at 20:16, on Zulip):

a LOT though :P

Santiago Pastorino (Apr 15 2019 at 13:46, on Zulip):

@oli you around?

Santiago Pastorino (Apr 15 2019 at 13:47, on Zulip):

1. https://github.com/spastorino/rust/commit/38e66f7397599d48a78aebcf23e2d4fb3485a9a3
2. https://github.com/spastorino/rust/commit/fc3faa5d7def51227fae05ae43e1de0e397a05c1
3. https://github.com/spastorino/rust/commit/d308b77860c6fced35cf43b7417b12648e503271

were you able to check all these?

Santiago Pastorino (Apr 15 2019 at 13:47, on Zulip):

I have more stuff locally

oli (Apr 15 2019 at 13:48, on Zulip):

sorry, didn't see it and I gotta run in a minute

Santiago Pastorino (Apr 15 2019 at 13:49, on Zulip):

no worries

Santiago Pastorino (Apr 15 2019 at 13:49, on Zulip):

when you can let me know

Santiago Pastorino (Apr 15 2019 at 13:49, on Zulip):

I'm not blocked or anything like that

Santiago Pastorino (Apr 15 2019 at 13:54, on Zulip):

1. https://github.com/spastorino/rust/commit/38e66f7397599d48a78aebcf23e2d4fb3485a9a3
2. https://github.com/spastorino/rust/commit/fc3faa5d7def51227fae05ae43e1de0e397a05c1
3. https://github.com/spastorino/rust/commit/d308b77860c6fced35cf43b7417b12648e503271
4. https://github.com/spastorino/rust/commit/2d800f96b43dded0484a68f9703cd056c8e06464

Santiago Pastorino (Apr 15 2019 at 13:55, on Zulip):

I think you've already checked 1

Santiago Pastorino (Apr 15 2019 at 18:20, on Zulip):

@oli one question ...

Santiago Pastorino (Apr 15 2019 at 18:20, on Zulip):

I'm getting a lot of Place expected Option found errors

Santiago Pastorino (Apr 15 2019 at 18:21, on Zulip):

thing is when you are in a projection and do base to go up

Santiago Pastorino (Apr 15 2019 at 18:21, on Zulip):

but the problem with that is that I need the base inside projection if it's Some(..)

Santiago Pastorino (Apr 15 2019 at 18:21, on Zulip):

otherwise I need the PlaceBase

Santiago Pastorino (Apr 15 2019 at 18:22, on Zulip):

I was thinking about doing a method for that but the problem is that those are different types

Santiago Pastorino (Apr 15 2019 at 18:23, on Zulip):

and if I don't solve that in this way I'd need to check if base is Some otherwise go to the top base

Santiago Pastorino (Apr 15 2019 at 18:23, on Zulip):

gonna do that for now but those may end being a lot of changes to rollback :/

Santiago Pastorino (Apr 15 2019 at 18:26, on Zulip):

for instance base here https://github.com/rust-lang/rust/blob/2d800f96b43dded0484a68f9703cd056c8e06464/src/librustc_mir/borrow_check/mod.rs#L1750-L1757

Santiago Pastorino (Apr 15 2019 at 18:27, on Zulip):

and on top of that, next problem is that https://github.com/rust-lang/rust/blob/2d800f96b43dded0484a68f9703cd056c8e06464/src/librustc_mir/borrow_check/mod.rs#L1751 takes a Place but I don't have a Place anymore

Santiago Pastorino (Apr 15 2019 at 18:27, on Zulip):

I wonder if we need to define a trait to solve that

oli (Apr 15 2019 at 18:32, on Zulip):

hmm... I was thinking that we should pass down the PlaceBase wherever the final None needs a PlaceBase

oli (Apr 15 2019 at 18:32, on Zulip):

(or convert whatever is happening to use the unroll code)

Santiago Pastorino (Apr 15 2019 at 18:33, on Zulip):

didn't understand what you meant

Santiago Pastorino (Apr 15 2019 at 18:33, on Zulip):

you were thinking that instead of passing down base, we should pass everything? included the PlaceBase?

oli (Apr 15 2019 at 18:34, on Zulip):

So if I understand you correctly, after traversing the "Projection-Linked-List", when you end up with the None, that's where you used to have the Place::Base instead of another Place::Projection

Santiago Pastorino (Apr 15 2019 at 18:35, on Zulip):

let me explain it simpler

oli (Apr 15 2019 at 18:35, on Zulip):

In order to achieve the same behaviour, whatever code matches on the proj.base field, needs to also know about the actual PlaceBase that the Place has

Santiago Pastorino (Apr 15 2019 at 18:35, on Zulip):

https://github.com/rust-lang/rust/blob/2d800f96b43dded0484a68f9703cd056c8e06464/src/librustc_mir/borrow_check/mod.rs#L1750-L1757

Santiago Pastorino (Apr 15 2019 at 18:35, on Zulip):

ok, so you meant that I should pass to those methods both things?

Santiago Pastorino (Apr 15 2019 at 18:36, on Zulip):
                             self.check_if_full_path_is_moved(
                                 context, InitializationRequiringAction::Use,
                                 (place.base, base, span), flow_state);
Santiago Pastorino (Apr 15 2019 at 18:36, on Zulip):

something like this?

oli (Apr 15 2019 at 18:38, on Zulip):

yes that. It's essentially what was passed down before

Santiago Pastorino (Apr 15 2019 at 18:38, on Zulip):

yep

Santiago Pastorino (Apr 15 2019 at 18:38, on Zulip):

ok

Santiago Pastorino (Apr 15 2019 at 18:41, on Zulip):

place.base is private :(

Santiago Pastorino (Apr 15 2019 at 18:41, on Zulip):

wonder if I should do those crate visible or just expose methods

Santiago Pastorino (Apr 15 2019 at 18:50, on Zulip):

@oli I wonder what about ...

Santiago Pastorino (Apr 15 2019 at 18:51, on Zulip):
self.check_if_full_path_is_moved(
    context, InitializationRequiringAction::Use,
    (&Place {
        base,
        projection: *proj_base,
    }, span), flow_state);
oli (Apr 15 2019 at 19:02, on Zulip):

that constructs new places, which is probably very expensive (boxes and such)

oli (Apr 15 2019 at 19:03, on Zulip):

Most of this stuff will in the future get a base and a slice of the rest of the projections

oli (Apr 15 2019 at 19:03, on Zulip):

so right now it needs the base and the "linked list" of projections

Santiago Pastorino (Apr 15 2019 at 19:05, on Zulip):

:+1:

Santiago Pastorino (Apr 15 2019 at 19:06, on Zulip):

the other kind of recurrent thing I'm seeing is the thing with the type

Santiago Pastorino (Apr 15 2019 at 19:07, on Zulip):

getting the type of the projection or the type of the base if the proj is None

Santiago Pastorino (Apr 15 2019 at 19:07, on Zulip):

I guess I can add those 4 or 5 lines of code everywhere but ... hmmm

oli (Apr 15 2019 at 19:53, on Zulip):

hmm... maybe make the ProjectionElem::ty method take a PlaceBase, too?

Santiago Pastorino (Apr 15 2019 at 22:27, on Zulip):

@oli that's weird from my point of view

Santiago Pastorino (Apr 15 2019 at 22:30, on Zulip):

wouldn't be better to have something like Place::ty(base: &PlaceBase, elem: &ProjectionElem)?

oli (Apr 16 2019 at 07:39, on Zulip):

sgtm

Santiago Pastorino (Apr 16 2019 at 18:05, on Zulip):

@oli one thing that I wonder is ... we are going to start passing down as we said yesterday PlaceBase and PlaceProjection

Santiago Pastorino (Apr 16 2019 at 18:05, on Zulip):

I guess we need to pass down Option<Box<PlaceProjection>> really

Santiago Pastorino (Apr 16 2019 at 18:05, on Zulip):

and wonder if we should have a type alias for that or something

Santiago Pastorino (Apr 16 2019 at 18:06, on Zulip):

I'm probably going to take one path and see how goes but if you know beforehand you could save me some time :D

Santiago Pastorino (Apr 16 2019 at 18:06, on Zulip):

going to pass the Option<Box<PlaceProjection>> down

Santiago Pastorino (Apr 16 2019 at 18:45, on Zulip):

@oli I also keep wordering if it's not easier to build a Place back

Santiago Pastorino (Apr 16 2019 at 18:46, on Zulip):

I know it's problematic because of performance

oli (Apr 16 2019 at 18:46, on Zulip):

yea, I'd rather not do that

Santiago Pastorino (Apr 16 2019 at 18:46, on Zulip):

but I think it may be easier as an intermediate step

Santiago Pastorino (Apr 16 2019 at 18:46, on Zulip):

I mean, just a previous commit in the same PR

oli (Apr 16 2019 at 18:46, on Zulip):

if you can get rid of it within the same PR, sure

Santiago Pastorino (Apr 16 2019 at 18:46, on Zulip):

this is a massive change

Santiago Pastorino (Apr 16 2019 at 18:47, on Zulip):

what do you think? :)

Santiago Pastorino (Apr 16 2019 at 18:47, on Zulip):

your idea was ... as I saw saying a bit before, to pass PlaceBase and Option<Box<PlaceProjection>> all the way down?

oli (Apr 16 2019 at 18:47, on Zulip):

as long as at the end of the PR we don't have any temporary Places and allocations that are created and thrown away, as an intermediate step that's perfectly alright

Santiago Pastorino (Apr 16 2019 at 18:48, on Zulip):

ok

Santiago Pastorino (Apr 16 2019 at 18:48, on Zulip):

and if I follow the other approach

Santiago Pastorino (Apr 16 2019 at 18:48, on Zulip):

your idea was ... as I saw saying a bit before, to pass PlaceBase and Option<Box<PlaceProjection>> all the way down?

this :point_up: was the idea, right?

oli (Apr 16 2019 at 18:49, on Zulip):

yes

oli (Apr 16 2019 at 18:50, on Zulip):

I mean you can use a type alias or a new type or even make Place generic over AsRef<PlaceProjection> or sth if that is more usable

Santiago Pastorino (Apr 17 2019 at 14:26, on Zulip):

@oli was going over your review

Santiago Pastorino (Apr 17 2019 at 14:26, on Zulip):

https://github.com/spastorino/rust/commit/d308b77860c6fced35cf43b7417b12648e503271#commitcomment-33214690

Santiago Pastorino (Apr 17 2019 at 14:26, on Zulip):

I guess PlaceBase can't be Copy because we want the Box

oli (Apr 17 2019 at 14:27, on Zulip):

oh right

Santiago Pastorino (Apr 17 2019 at 16:37, on Zulip):

@oli about https://github.com/spastorino/rust/commit/2d800f96b43dded0484a68f9703cd056c8e06464#r33209896

Santiago Pastorino (Apr 17 2019 at 16:38, on Zulip):

I think we need to merge the iter thing first, right?

Santiago Pastorino (Apr 17 2019 at 16:38, on Zulip):

seems like we are in the need of iterating over in several parts

Santiago Pastorino (Apr 18 2019 at 01:18, on Zulip):

@oli in case you agree we need to merge the iterator thing first, here is the thing https://github.com/rust-lang/rust/pull/60063

Santiago Pastorino (Apr 18 2019 at 01:18, on Zulip):

don't remember if you wanted something else in that PR

Santiago Pastorino (Apr 18 2019 at 01:19, on Zulip):

we could migrate more things to use iter(), implement things in terms of that and then in the struct thing just change the iter() impl and get along with it

Santiago Pastorino (Apr 18 2019 at 01:19, on Zulip):

that was your idea?

Santiago Pastorino (Apr 18 2019 at 01:19, on Zulip):

can you give one example of how that would look like?

oli (Apr 18 2019 at 12:13, on Zulip):

The iterator PR lgtm, we can merge that as it is. I was thinking of the Debug impl of Place, the original code recursed on Place::Projection, basically the idea is to get rid of the recursion and use your new iterate method

oli (Apr 18 2019 at 12:14, on Zulip):

r=me on the PR or if you want you can already experiment with porting the Debug impl in the same PR, but I'm fine with it either way

Santiago Pastorino (Apr 18 2019 at 12:51, on Zulip):

@oli I guess is better if you merge that

Santiago Pastorino (Apr 18 2019 at 12:51, on Zulip):

so I can rebase the other branch on top

Santiago Pastorino (Apr 18 2019 at 12:51, on Zulip):

and do that in the next PR

Santiago Pastorino (Apr 18 2019 at 12:52, on Zulip):

the best thing of that is meanwhile I'm on vacations that is going to land

Santiago Pastorino (Apr 18 2019 at 12:52, on Zulip):

unsure what do you mean by r=me

Santiago Pastorino (Apr 18 2019 at 12:52, on Zulip):

https://github.com/rust-lang/rust/pull/60063

Santiago Pastorino (Apr 18 2019 at 12:52, on Zulip):

isn't what I did enough?

oli (Apr 19 2019 at 07:54, on Zulip):

unsure what do you mean by r=me

That's bors-speak for anyone with bors powers may write @bors r=oli-obk so the PR gets merged but is marked as approved by me

Santiago Pastorino (Apr 19 2019 at 11:08, on Zulip):

ahh yeah I don’t have bors rights, @nikomatsakis told me that I should have 😊

Saleem Jaffer (Apr 24 2019 at 05:29, on Zulip):

(deleted)

Santiago Pastorino (Apr 25 2019 at 05:13, on Zulip):

@oli you were planning to implement Debug for Place using iterate if I'm correct

Santiago Pastorino (Apr 25 2019 at 05:13, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/mir/mod.rs#L2157-L2215

Santiago Pastorino (Apr 25 2019 at 05:13, on Zulip):

how were you planning to do so?

Santiago Pastorino (Apr 25 2019 at 05:14, on Zulip):

quickly looking at the code the only way I can think of is using a Stack

Santiago Pastorino (Apr 25 2019 at 05:18, on Zulip):

because for each projection I need to print a starting part, then continue to the next projection and then when I'm back I print the last part

Santiago Pastorino (Apr 25 2019 at 05:19, on Zulip):

or I guess I could iterate forward, print the opening part, go to base print it and iterate again in reverse to print the closing parts

Santiago Pastorino (Apr 25 2019 at 12:29, on Zulip):

@oli you there?

oli (Apr 25 2019 at 12:29, on Zulip):

jup

Santiago Pastorino (Apr 25 2019 at 12:30, on Zulip):

can you explain why this https://github.com/rust-lang/rust/pull/60247/files#r278458354 ?

Santiago Pastorino (Apr 25 2019 at 12:30, on Zulip):

first thing

Santiago Pastorino (Apr 25 2019 at 12:30, on Zulip):

for instance for a.b.c

Santiago Pastorino (Apr 25 2019 at 12:30, on Zulip):

old repr was .c -> .b -> .a

Santiago Pastorino (Apr 25 2019 at 12:30, on Zulip):

isn't the new one the same?

oli (Apr 25 2019 at 12:32, on Zulip):

the new one is (a, .c -> .b -> None), but the iterator specifically exists to iterate in "reverse" order (reverse when lookng at the recursive datastructure). so iterating you kinda get (a, [.b, .c]) (assuming the slice in here is an iterator)

Santiago Pastorino (Apr 25 2019 at 12:44, on Zulip):

yeah, so by doing iterating directly, then going to base and then iterating backwards I'm doing .c -> .b -> .a -> .b -> .c

Santiago Pastorino (Apr 25 2019 at 12:44, on Zulip):

isn't that an exact replica of the recursion?

oli (Apr 25 2019 at 12:45, on Zulip):

yes, but we're trying to replicate the output here in a way that once the recursive datastructure is gone, will require minimal changes

oli (Apr 25 2019 at 12:46, on Zulip):

Basically your PR that turns Place into a struct should not have to touch the Debug impl after this change

oli (Apr 25 2019 at 12:46, on Zulip):

oh I misunderstdood your question

Santiago Pastorino (Apr 25 2019 at 12:46, on Zulip):

ahh sorry, I think the iterator gives you .b -> .c

Santiago Pastorino (Apr 25 2019 at 12:46, on Zulip):

not .c -> .b

Santiago Pastorino (Apr 25 2019 at 12:47, on Zulip):

right?

oli (Apr 25 2019 at 12:47, on Zulip):

yes, that's what I was failing to explain

Santiago Pastorino (Apr 25 2019 at 12:47, on Zulip):

I'm not looking at the code and didn't look at the playing by memory

Santiago Pastorino (Apr 25 2019 at 12:47, on Zulip):

:+1:

Santiago Pastorino (Apr 25 2019 at 12:47, on Zulip):

just force pushed

oli (Apr 25 2019 at 12:48, on Zulip):

jup, that looks right to me now

Santiago Pastorino (Apr 25 2019 at 12:48, on Zulip):

as soon as the ci is happy is ready to merge then

Santiago Pastorino (Apr 25 2019 at 12:51, on Zulip):

yesterday after a some days off this work come back to it and rebased on top of the iterate changes

Santiago Pastorino (Apr 25 2019 at 12:51, on Zulip):

now did this and going to sit the WIP thing we have been talking about on top of also this PR

Santiago Pastorino (Apr 25 2019 at 12:51, on Zulip):

so I can continue with the rest

oli (Apr 25 2019 at 12:56, on Zulip):

It's so cool that you're doing all this work. I hope to soonish have more time, so that mir-opt actually starts deserving the term working group

Santiago Pastorino (Apr 25 2019 at 12:58, on Zulip):

:)

Santiago Pastorino (Apr 25 2019 at 12:59, on Zulip):

I wish I can finish this quickly so I can involve in other things too

Santiago Pastorino (Apr 25 2019 at 14:44, on Zulip):

@oli https://github.com/rust-lang/rust/pull/60247 ready to r+ :)

Santiago Pastorino (Apr 29 2019 at 16:22, on Zulip):

@oli I've rebased the struct thing a while ago

Santiago Pastorino (Apr 29 2019 at 16:22, on Zulip):

squashed everything

Santiago Pastorino (Apr 29 2019 at 16:22, on Zulip):

and continued the work going

Santiago Pastorino (Apr 29 2019 at 16:22, on Zulip):

I'm not sure if I should keep going and provide a huge commit or do you want to keep seeing this WIP X commits

Santiago Pastorino (Apr 29 2019 at 16:23, on Zulip):

anyway right now everything is amended with more stuff but I can do a WIP 1 thing and keep that going until this compiles

oli (Apr 29 2019 at 18:10, on Zulip):

I'm fine with you squashing it

Santiago Pastorino (Apr 29 2019 at 18:14, on Zulip):

ok

Santiago Pastorino (Apr 29 2019 at 18:14, on Zulip):

man it's a lot of work

Santiago Pastorino (Apr 29 2019 at 18:14, on Zulip):

I mean, it's easy but it's a lot of stuff :P

Santiago Pastorino (Apr 29 2019 at 18:14, on Zulip):

I'm right now seeing 322 errors

Santiago Pastorino (Apr 29 2019 at 18:14, on Zulip):

changes are not that mechanical really

Santiago Pastorino (Apr 29 2019 at 18:15, on Zulip):

maybe we can set a checkpoint at some given time so you can do a review

Santiago Pastorino (Apr 29 2019 at 18:15, on Zulip):

I'm afraid of spending a huge amount of hours doing something that is not exactly what you imagined

oli (Apr 29 2019 at 18:15, on Zulip):

I'm sitting on a train right now

oli (Apr 29 2019 at 18:15, on Zulip):

throw stuff at me

Santiago Pastorino (Apr 29 2019 at 18:15, on Zulip):

ok

Santiago Pastorino (Apr 29 2019 at 18:15, on Zulip):

can push what I have now

oli (Apr 29 2019 at 18:15, on Zulip):

:+1:

Santiago Pastorino (Apr 29 2019 at 18:16, on Zulip):

I think there were already some pending comments on your side

Santiago Pastorino (Apr 29 2019 at 18:16, on Zulip):

have those written down

Santiago Pastorino (Apr 29 2019 at 18:16, on Zulip):

https://github.com/spastorino/rust/commit/8b96364a4caf4280c3ab02d539644d557a4ba748

Santiago Pastorino (Apr 29 2019 at 18:17, on Zulip):

@oli ^^^, a couple of comments before you doing a review :)

Santiago Pastorino (Apr 29 2019 at 18:17, on Zulip):

there's Place::ty_from, I've added that but didn't make all the code use that

Santiago Pastorino (Apr 29 2019 at 18:17, on Zulip):

I'd need to remove the repetitions and replace with a call to this

Santiago Pastorino (Apr 29 2019 at 18:17, on Zulip):

and there's also a lot of &Place { } I was going to add a FIXME before each of those but didn't do that yet neither

Santiago Pastorino (Apr 29 2019 at 18:18, on Zulip):

I'm not even sure if doing that is possible or a good idea yet

Santiago Pastorino (Apr 29 2019 at 18:18, on Zulip):

will see

Santiago Pastorino (Apr 29 2019 at 18:18, on Zulip):

I fear I may end in a lot of used of moved value errors

Santiago Pastorino (Apr 29 2019 at 18:18, on Zulip):

I may need to clone things that are inside

oli (Apr 29 2019 at 18:18, on Zulip):

can you elaborate on the clone thing?

Santiago Pastorino (Apr 29 2019 at 18:18, on Zulip):

anyway, it's a temporary hack to avoid changing all the signatures of the functions I'm calling

oli (Apr 29 2019 at 18:19, on Zulip):

temporary as in will be gone by the end of the PR?

Santiago Pastorino (Apr 29 2019 at 18:19, on Zulip):

yes

Santiago Pastorino (Apr 29 2019 at 18:19, on Zulip):

I want to have something as minimal as possible compiling

oli (Apr 29 2019 at 18:19, on Zulip):

ah, ok, that's fine then

Santiago Pastorino (Apr 29 2019 at 18:19, on Zulip):

so I can run tests and be sure

Santiago Pastorino (Apr 29 2019 at 18:19, on Zulip):

changing all the functions signatures to receive base and projection instead of place would be a lot of work I think

Santiago Pastorino (Apr 29 2019 at 18:20, on Zulip):

so more changes all over the place that may introduce changes

oli (Apr 29 2019 at 18:21, on Zulip):

right

oli (Apr 29 2019 at 18:21, on Zulip):

so at first you're just creating a new Place?

Santiago Pastorino (Apr 29 2019 at 18:23, on Zulip):

unsure what do you meant

Santiago Pastorino (Apr 29 2019 at 18:24, on Zulip):

if you're talking about those cases, yeah, I'm creating a new place out of a PlaceBase and a PlaceProjection

oli (Apr 29 2019 at 18:24, on Zulip):

you clone Places, modify the projection and then call the appropriate method, compared to what used to be just fetching the inner projection's place?

oli (Apr 29 2019 at 18:24, on Zulip):

gotcha

oli (Apr 29 2019 at 18:24, on Zulip):

do you think it would be less work to make Place generic and use a Place<Box<Projection>> vs a Place<&Projection> depending on the use case?

oli (Apr 29 2019 at 18:25, on Zulip):

the generic parameter could still default to Place<Box<Projection>> so most code doesn't have to be changed

Santiago Pastorino (Apr 29 2019 at 18:26, on Zulip):

sorry but I'm having a hard time following you :P

Santiago Pastorino (Apr 29 2019 at 18:26, on Zulip):

you clone Places, modify the projection and then call the appropriate method, compared to what used to be just fetching the inner projection's place?

unsure what do you meant :)

Santiago Pastorino (Apr 29 2019 at 18:26, on Zulip):

do you think it would be less work to make Place generic and use a Place<Box<Projection>> vs a Place<&Projection> depending on the use case?

unsure either

oli (Apr 29 2019 at 18:27, on Zulip):

so when the old code was recursing on a &Place, that was easy. You match on the variant, if it's a Projection, you get the inner Place and recurse

Santiago Pastorino (Apr 29 2019 at 18:27, on Zulip):

yes

oli (Apr 29 2019 at 18:27, on Zulip):

now, when you recurse, what you do is you fetch the projection field, match on that, build a new Place with the inner projection and the original base

Santiago Pastorino (Apr 29 2019 at 18:28, on Zulip):

yes

oli (Apr 29 2019 at 18:28, on Zulip):

this requires cloning the Box<Projection>, because you only have borrowed access

Santiago Pastorino (Apr 29 2019 at 18:28, on Zulip):

yes

oli (Apr 29 2019 at 18:30, on Zulip):

So instead of passing down the base and projection field separately, you could have a type almost like Place, but instead of the projection field being Box<Projection> it's &Projection

oli (Apr 29 2019 at 18:31, on Zulip):

in order to reduce code duplication, we can make Place generic over the projection field's type and either use Box<Projection> or &Projection depending on what the code does

Santiago Pastorino (Apr 29 2019 at 18:31, on Zulip):

ahh yes, I see what you meant

oli (Apr 29 2019 at 18:32, on Zulip):

so my question to you is: do you think this would help or is that even more churn?

oli (Apr 29 2019 at 18:32, on Zulip):

especially since we'll essentially be undoing it once we go to interned slices

Santiago Pastorino (Apr 29 2019 at 18:32, on Zulip):

yeah, I'm wondering about it

Santiago Pastorino (Apr 29 2019 at 18:32, on Zulip):

unsure

Santiago Pastorino (Apr 29 2019 at 18:33, on Zulip):

what do you think?

Santiago Pastorino (Apr 29 2019 at 18:33, on Zulip):

about the approach you have proposed

Santiago Pastorino (Apr 29 2019 at 18:33, on Zulip):

vs

Santiago Pastorino (Apr 29 2019 at 18:33, on Zulip):

just doing clone and keep passing Place

Santiago Pastorino (Apr 29 2019 at 18:33, on Zulip):

and then changing all the involved functions to receive both things

Santiago Pastorino (Apr 29 2019 at 18:34, on Zulip):

with your new approach you also want to avoid the functions receive both things?

oli (Apr 29 2019 at 18:34, on Zulip):

well, they'll still change the type of the argument they take, but the rest of the code should stay mostly unaffected

Santiago Pastorino (Apr 29 2019 at 18:34, on Zulip):

but once we go to interned slices, wouldn't we also need to receive both things in most of the functions?

oli (Apr 29 2019 at 18:34, on Zulip):

you could have a sort of as_ref method on Place that gives the "borrowed view"

Santiago Pastorino (Apr 29 2019 at 18:35, on Zulip):

but I don't need the borrowed view of an existing place

oli (Apr 29 2019 at 18:35, on Zulip):

well, yes, but if you have a Place you have both PlaceBase and &[Projection] and you can cheaply create a new place because slices (and subslices) are Copy

Santiago Pastorino (Apr 29 2019 at 18:35, on Zulip):

I need to strip the head projections and form a place from there

Santiago Pastorino (Apr 29 2019 at 18:36, on Zulip):

well, yes, but if you have a Place you have both PlaceBase and &[Projection] and you can cheaply create a new place because slices (and subslices) are Copy

true

oli (Apr 29 2019 at 18:36, on Zulip):

which would just be Place { base: old.base, proj: &old.proj[1..] }

Santiago Pastorino (Apr 29 2019 at 18:36, on Zulip):

so better to avoid changing all the signatures

Santiago Pastorino (Apr 29 2019 at 18:36, on Zulip):

hmm well seems like your idea is better then

oli (Apr 29 2019 at 18:36, on Zulip):

you'd still somewhat change the signatures

Santiago Pastorino (Apr 29 2019 at 18:36, on Zulip):

yes

Santiago Pastorino (Apr 29 2019 at 18:37, on Zulip):

but it's a minor change

oli (Apr 29 2019 at 18:37, on Zulip):

since you need to use Place<'tcx, &'a Projection> instead of Place<'tcx>

Santiago Pastorino (Apr 29 2019 at 18:37, on Zulip):

yep, agree but that changes is easier and simpler than receiving two things

Santiago Pastorino (Apr 29 2019 at 18:38, on Zulip):

and impls could stay the same

Santiago Pastorino (Apr 29 2019 at 18:38, on Zulip):

and in my case I need to adapt everything

Santiago Pastorino (Apr 29 2019 at 19:48, on Zulip):

@oli I saw you left https://github.com/spastorino/rust/commit/8b96364a4caf4280c3ab02d539644d557a4ba748#r33350775

Santiago Pastorino (Apr 29 2019 at 19:48, on Zulip):

did you reviewed the thing completely?

Santiago Pastorino (Apr 29 2019 at 19:48, on Zulip):

that's the only comment? :)

oli (Apr 29 2019 at 19:48, on Zulip):

yea, and immediately realized I wasn't sure if you wanted me to start reviewing and thus stopped

oli (Apr 29 2019 at 19:49, on Zulip):

then I forgot to tell you about that

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

hehe

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

so about that comment in particular, is wrong :)

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

I guess you confused something?

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

guess you want *projection

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

but mainly you wanted me to remove the ref?

oli (Apr 29 2019 at 19:49, on Zulip):

ah

Santiago Pastorino (Apr 29 2019 at 19:49, on Zulip):

also, about the review, please do so

oli (Apr 29 2019 at 19:49, on Zulip):

no, just don't do anything to projection

oli (Apr 29 2019 at 19:50, on Zulip):

if you match on a reference, you automatically get ref patterns

oli (Apr 29 2019 at 19:50, on Zulip):

match ergonomics are cool like that

oli (Apr 29 2019 at 19:50, on Zulip):

I just didn't realize you already had a reference

Santiago Pastorino (Apr 29 2019 at 19:50, on Zulip):

ahh yeah

Santiago Pastorino (Apr 29 2019 at 19:50, on Zulip):

I guess I need to check that RFC again

Santiago Pastorino (Apr 29 2019 at 19:51, on Zulip):

I'm never sure when that happens

Santiago Pastorino (Apr 29 2019 at 19:52, on Zulip):

maybe, let me push what I have now after this change and I guess you can review

Santiago Pastorino (Apr 29 2019 at 19:54, on Zulip):

@oli https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81

Santiago Pastorino (Apr 29 2019 at 19:55, on Zulip):

I guess what I'm going to do now is trying to do Place's Projection generic

Santiago Pastorino (Apr 29 2019 at 20:12, on Zulip):

the generic parameter could still default to Place<Box<Projection>> so most code doesn't have to be changed

what do you mean by this?

Santiago Pastorino (Apr 29 2019 at 20:13, on Zulip):

@oli how can I make a generic parameter default to a specific type?

oli (Apr 29 2019 at 20:13, on Zulip):

struct Foo<T = Bar>

oli (Apr 29 2019 at 20:13, on Zulip):

might need feature gates (if not already on)

Santiago Pastorino (Apr 29 2019 at 20:13, on Zulip):

ahh ok ok

Santiago Pastorino (Apr 29 2019 at 20:44, on Zulip):

@oli you around?

Santiago Pastorino (Apr 29 2019 at 20:44, on Zulip):

about this https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352321

Santiago Pastorino (Apr 29 2019 at 20:44, on Zulip):

wouldn't we want to use iterate?

Santiago Pastorino (Apr 29 2019 at 20:45, on Zulip):

in particular I was debating in a lot of places how much to change the code in the first commit

Santiago Pastorino (Apr 29 2019 at 20:45, on Zulip):

or just make the minimal changes as a low risk of getting failures and then refactor the methods

Santiago Pastorino (Apr 29 2019 at 20:45, on Zulip):

but yeah I guess I can do everything at once :P

oli (Apr 29 2019 at 20:46, on Zulip):

hmm... good point

Santiago Pastorino (Apr 29 2019 at 20:46, on Zulip):

about iterate?

oli (Apr 29 2019 at 20:46, on Zulip):

about not changing too much

Santiago Pastorino (Apr 29 2019 at 20:46, on Zulip):

ahh ya

Santiago Pastorino (Apr 29 2019 at 20:46, on Zulip):

that's why most of the code may look very bad :P

oli (Apr 29 2019 at 20:46, on Zulip):

that's fine

oli (Apr 29 2019 at 20:47, on Zulip):

you're right

oli (Apr 29 2019 at 20:47, on Zulip):

it's much easier to review this way

oli (Apr 29 2019 at 20:47, on Zulip):

I'll still leave the comments, but just keep them for addressing after everything works in the current way

Santiago Pastorino (Apr 29 2019 at 20:49, on Zulip):

I guess I should add a lot of FIXME SOMECODE and message

Santiago Pastorino (Apr 29 2019 at 20:49, on Zulip):

like

Santiago Pastorino (Apr 29 2019 at 20:49, on Zulip):

// FIXME PLACE2 Make this code use iterate

Santiago Pastorino (Apr 29 2019 at 20:49, on Zulip):

something like that

Santiago Pastorino (Apr 29 2019 at 20:50, on Zulip):

in particular, now I was thinking, should that code use iterate?

Santiago Pastorino (Apr 29 2019 at 20:51, on Zulip):

the problem is that iterate gives me the thing in the normal order but I need it in the reverse order

Santiago Pastorino (Apr 29 2019 at 20:51, on Zulip):

actually, most of the time I need the stuff in reverse order, right?

Santiago Pastorino (Apr 29 2019 at 20:52, on Zulip):

I'm not sure why iterate is giving the thing in the straight order

oli (Apr 29 2019 at 21:05, on Zulip):

it's 50/50

oli (Apr 29 2019 at 21:06, on Zulip):

you can create a second iterate_rev function that iterates directly for the cases that iterate in the reverse order and implement these algorithms on top of that if you want, but as a first step using a loop should work out well enough

Santiago Pastorino (Apr 29 2019 at 21:09, on Zulip):

yeah, so with this current impl we can't make a DoubleEndedIterator we could when we use the sliced version

Santiago Pastorino (Apr 29 2019 at 21:10, on Zulip):

we need to have a complete separate implementation of the reverse iterator

oli (Apr 29 2019 at 21:10, on Zulip):

yea

Santiago Pastorino (Apr 29 2019 at 21:10, on Zulip):

btw: https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352504

oli (Apr 29 2019 at 21:10, on Zulip):

unfortunate, but as you said, we can fix that in the future

Santiago Pastorino (Apr 29 2019 at 21:11, on Zulip):

what do you want me to do there?

oli (Apr 29 2019 at 21:12, on Zulip):

ah ups, answered on the comment

oli (Apr 29 2019 at 21:12, on Zulip):

I don't see how the methods are ever more concise than directly accessing the field

Santiago Pastorino (Apr 29 2019 at 21:15, on Zulip):

I also have some doubts about this https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352803

Santiago Pastorino (Apr 29 2019 at 21:16, on Zulip):

I don't see how the methods are ever more concise than directly accessing the field

I can't access the fields, unless we want to make them public

Santiago Pastorino (Apr 29 2019 at 21:16, on Zulip):

which may make sense :)

oli (Apr 29 2019 at 21:17, on Zulip):

oh lol

oli (Apr 29 2019 at 21:17, on Zulip):

I did not see that

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

and about this https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352803

oli (Apr 29 2019 at 21:17, on Zulip):

yea, just make them public, the enum variants were public, too

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

yes

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

agreed

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

about this https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352803

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

it's wrong yeah

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

we want projection be none and base be local

Santiago Pastorino (Apr 29 2019 at 21:17, on Zulip):

that's not equivalent

oli (Apr 29 2019 at 21:18, on Zulip):

seems like an easy footgun, maybe just remove the method in favour of a method on PlaceBase?

Santiago Pastorino (Apr 29 2019 at 21:18, on Zulip):

the local function is "similar" but it considers Deref too

oli (Apr 29 2019 at 21:19, on Zulip):

yea, but that one was preexisting, right?

Santiago Pastorino (Apr 29 2019 at 21:19, on Zulip):

yes

Santiago Pastorino (Apr 29 2019 at 21:19, on Zulip):

and yes

Santiago Pastorino (Apr 29 2019 at 21:19, on Zulip):

place.base.local()

Santiago Pastorino (Apr 29 2019 at 21:19, on Zulip):

seems better

oli (Apr 29 2019 at 21:22, on Zulip):

so I'm done with my review pass

Santiago Pastorino (Apr 29 2019 at 21:22, on Zulip):

:+1:

Santiago Pastorino (Apr 29 2019 at 21:22, on Zulip):

I'm probably going to stop for today

Santiago Pastorino (Apr 29 2019 at 21:22, on Zulip):

next steps I'm going to fix your comments and migrate to a generic Place

Santiago Pastorino (Apr 29 2019 at 21:23, on Zulip):

then keep fixing errors

Santiago Pastorino (Apr 29 2019 at 21:23, on Zulip):

when I see the amount of errors to fix I want cry :P

oli (Apr 29 2019 at 21:31, on Zulip):

I feel you. The fallout from touching omnipresent datatypes like that is insane

Santiago Pastorino (Apr 29 2019 at 21:37, on Zulip):

hehe, no worries :)

Santiago Pastorino (Apr 30 2019 at 14:21, on Zulip):

@oli back to this

Santiago Pastorino (Apr 30 2019 at 14:21, on Zulip):

about your comment here https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352380

oli (Apr 30 2019 at 14:21, on Zulip):

:wave:

Santiago Pastorino (Apr 30 2019 at 14:22, on Zulip):

we are using base to return StorageDeadOrDrop::LocalStorageDead

oli (Apr 30 2019 at 14:22, on Zulip):

but you don't care which base

Santiago Pastorino (Apr 30 2019 at 14:22, on Zulip):

you just want to use if let else?

oli (Apr 30 2019 at 14:22, on Zulip):

this is just the projections: None case

Santiago Pastorino (Apr 30 2019 at 14:22, on Zulip):

is that what you meant?

oli (Apr 30 2019 at 14:23, on Zulip):

no, literally just do match place.projection

oli (Apr 30 2019 at 14:25, on Zulip):

if you look at the patterns, the base field is pretty much ignored (there's an exhaustive match for a single arm)

oli (Apr 30 2019 at 14:25, on Zulip):

this is a preexisting issue, just amplified by your change

Santiago Pastorino (Apr 30 2019 at 14:27, on Zulip):

yeah ok

Santiago Pastorino (Apr 30 2019 at 14:27, on Zulip):

I was doing an if let Some(...) = place.projection {} else {}

Santiago Pastorino (Apr 30 2019 at 14:27, on Zulip):

but match is better :)

oli (Apr 30 2019 at 14:29, on Zulip):

ah, that's what you meant. yea either is fine, but match has less churn since you don't need to swap the arms

Santiago Pastorino (Apr 30 2019 at 14:29, on Zulip):

yep :+1:

Santiago Pastorino (Apr 30 2019 at 14:31, on Zulip):

as you have probably seen I've been putting zero attention to the implementation of things

Santiago Pastorino (Apr 30 2019 at 14:31, on Zulip):

was more into make the minimal changes to make this compile and work

Santiago Pastorino (Apr 30 2019 at 14:31, on Zulip):

planned another pass over to refactor

Santiago Pastorino (Apr 30 2019 at 14:31, on Zulip):

but anyway, both ways work :)

Santiago Pastorino (Apr 30 2019 at 14:32, on Zulip):

just using the opportunity given that you gave some feedback to improve things in this same commit :)

Santiago Pastorino (Apr 30 2019 at 14:52, on Zulip):

@oli about https://github.com/spastorino/rust/commit/1a0142ca44944d6b624bc1bd9908304f34ce2f81#r33352587

Santiago Pastorino (Apr 30 2019 at 14:52, on Zulip):

most of the changes are related to indentation

Santiago Pastorino (Apr 30 2019 at 14:52, on Zulip):

anyway, I think your changes look better

Santiago Pastorino (Apr 30 2019 at 14:53, on Zulip):

probably better to do a while let

oli (Apr 30 2019 at 15:19, on Zulip):

yea, I should have probably made github ignore whitespace

Santiago Pastorino (May 02 2019 at 21:04, on Zulip):

@oli opened a very simple PR https://github.com/rust-lang/rust/pull/60486 with things that I've found along the way and had on my place 2.0 branch

Santiago Pastorino (May 02 2019 at 21:04, on Zulip):

but I think it's easier if we just merge that stuff using a separate PR

oli (May 03 2019 at 08:34, on Zulip):

good idea, easier to merge, easier to review and less likely to miss something during review :D

Santiago Pastorino (May 07 2019 at 16:09, on Zulip):

(deleted)

Santiago Pastorino (May 07 2019 at 16:10, on Zulip):

(deleted)

Santiago Pastorino (May 07 2019 at 16:10, on Zulip):

(deleted)

Santiago Pastorino (May 07 2019 at 16:10, on Zulip):

(deleted)

Santiago Pastorino (May 07 2019 at 16:20, on Zulip):

nevermind, what I was thinking is wrong

Santiago Pastorino (May 08 2019 at 17:04, on Zulip):

@oli you around?

Santiago Pastorino (May 08 2019 at 17:04, on Zulip):

I've reached a point where may be nice to review something

Santiago Pastorino (May 08 2019 at 17:06, on Zulip):

librustc_mir is compiling modulo some move out of borrowed content due to the silly Place temp, I've created, probably gonna go over what we talked about before of making PlaceProjection generic

oli (May 08 2019 at 18:48, on Zulip):

I won't have time today, but send me a link and I'll look at it tomorrow

Santiago Pastorino (May 10 2019 at 20:01, on Zulip):

@oli you around by any chance?

oli (May 11 2019 at 11:36, on Zulip):

am now

oli (May 11 2019 at 11:36, on Zulip):

@Santiago Pastorino

Santiago Pastorino (May 13 2019 at 15:58, on Zulip):

@oli sorry I couldn't get to you

oli (May 13 2019 at 15:58, on Zulip):

XD no worries

Santiago Pastorino (May 13 2019 at 15:58, on Zulip):

ended passing both place_base and place_projection everywhere

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

didn't go the generic way

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

well basically doing that now :)

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

passing both things

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

building a Place in place :) is not a good idea

oli (May 13 2019 at 15:59, on Zulip):

yeaaa

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

but it's not a good idea even as an intermediate step :)

Santiago Pastorino (May 13 2019 at 15:59, on Zulip):

or well, may it is if I clone a lot of things

Santiago Pastorino (May 13 2019 at 16:00, on Zulip):

unsure

Santiago Pastorino (May 13 2019 at 16:00, on Zulip):

anyway, do you think that passing both things is ok?

oli (May 13 2019 at 16:00, on Zulip):

it's ok, not ideal, but if we merge right after beta cutoff, we have 6 weeks to "fix it" :D

Santiago Pastorino (May 13 2019 at 16:00, on Zulip):

hehe :)

Santiago Pastorino (May 13 2019 at 16:01, on Zulip):

what would be ideal for you?

Santiago Pastorino (May 13 2019 at 16:01, on Zulip):

I'm kind of in the middle of making progress and doing the right thing :)

Santiago Pastorino (May 13 2019 at 16:01, on Zulip):

of course the right thing is going to be the last picture of the work

oli (May 13 2019 at 16:01, on Zulip):

no I mean, there is no ideal solution for this intermediate situation

Santiago Pastorino (May 13 2019 at 16:02, on Zulip):

ahh ya

Santiago Pastorino (May 13 2019 at 16:02, on Zulip):

ok

Santiago Pastorino (May 13 2019 at 16:02, on Zulip):

I'd say, I'm going to change to receive both things and we can review the PR

Santiago Pastorino (May 13 2019 at 16:02, on Zulip):

and discuss from there

oli (May 13 2019 at 16:02, on Zulip):

:+1:

Santiago Pastorino (May 13 2019 at 16:07, on Zulip):

one question btw

Santiago Pastorino (May 13 2019 at 16:07, on Zulip):

for instance prefixes

Santiago Pastorino (May 13 2019 at 16:07, on Zulip):

which gives back an interator over Place

Santiago Pastorino (May 13 2019 at 16:07, on Zulip):

I wonder what to do exactly with this new setup

oli (May 13 2019 at 16:08, on Zulip):

do you have a link?

Santiago Pastorino (May 13 2019 at 16:08, on Zulip):

if I go over PlaceProjection I'm going to miss the base

Santiago Pastorino (May 13 2019 at 16:09, on Zulip):

https://github.com/rust-lang/rust/blob/0a71d47f464cd59907cfdd69c570d3cd693404a8/src/librustc_mir/borrow_check/prefixes.rs

Santiago Pastorino (May 13 2019 at 16:09, on Zulip):

but talking in general about that situation not about my code in particular because I haven't changed that yet :)

Santiago Pastorino (May 13 2019 at 16:12, on Zulip):

I guess in the cases where some code like this one wants to get projections and base I could use trait objects or something like that

Santiago Pastorino (May 13 2019 at 16:12, on Zulip):

but unsure what you had in mind

oli (May 13 2019 at 16:17, on Zulip):

hmm... looks like you can use the same trick with a base field and a projection field?

Santiago Pastorino (May 13 2019 at 16:18, on Zulip):

unsure what you meant

Santiago Pastorino (May 13 2019 at 16:18, on Zulip):

I'm mainly talking about what I'm going to iterate over

Santiago Pastorino (May 13 2019 at 16:18, on Zulip):

next will return Option<?>

oli (May 13 2019 at 16:18, on Zulip):

Ah

oli (May 13 2019 at 16:19, on Zulip):

hmm... just use a tuple over PlaceBase and PlaceProjection I guess?

oli (May 13 2019 at 16:19, on Zulip):

(reference to PlaceProjection?)

Santiago Pastorino (May 13 2019 at 16:19, on Zulip):

yes that works, seems bad to me :P

oli (May 13 2019 at 16:20, on Zulip):

or... just &PlaceProjection?

oli (May 13 2019 at 16:20, on Zulip):

the user can fetch the base from the original place

Santiago Pastorino (May 13 2019 at 16:20, on Zulip):

yes

Santiago Pastorino (May 13 2019 at 16:20, on Zulip):

that's always possible

Santiago Pastorino (May 13 2019 at 16:20, on Zulip):

unsure what are the uses exactly of prefixes

oli (May 13 2019 at 16:21, on Zulip):

idk :D

Santiago Pastorino (May 13 2019 at 16:21, on Zulip):

will try to go over placeprojection only then :)

oli (May 13 2019 at 16:21, on Zulip):

https://github.com/rust-lang/rust/blob/0a71d47f464cd59907cfdd69c570d3cd693404a8/src/librustc_mir/borrow_check/prefixes.rs#L62 could even take just an &Option<PlaceProjection> instead of a &Place

oli (May 13 2019 at 16:22, on Zulip):

or whatever the exact type of the projection field is nowadays ^^

oli (May 13 2019 at 16:22, on Zulip):

All of these options and boxes are slightly confusing

Santiago Pastorino (May 13 2019 at 16:51, on Zulip):

:+1:

Santiago Pastorino (May 21 2019 at 23:18, on Zulip):

@oli from our private chat https://github.com/rust-lang/rust/pull/61023

Santiago Pastorino (May 21 2019 at 23:23, on Zulip):

for the record

Santiago Pastorino (May 21 2019 at 23:24, on Zulip):

gonna keep sending this kind of PRs to move to iterate over recursion so the general PR https://github.com/rust-lang/rust/pull/60913 it's a bit easier to merge and work with

Santiago Pastorino (May 22 2019 at 03:56, on Zulip):

@oli also https://github.com/rust-lang/rust/pull/61030

Santiago Pastorino (May 22 2019 at 11:50, on Zulip):

@oli you around?

Santiago Pastorino (May 22 2019 at 11:50, on Zulip):

about https://github.com/rust-lang/rust/pull/61030/files#r286370409

Santiago Pastorino (May 22 2019 at 11:51, on Zulip):

to get rid of the continue I need to return None there but also add an else return None here https://github.com/rust-lang/rust/pull/61030/files#diff-7419efc1fe3c0f6f5e4231405f7c409eR403

Santiago Pastorino (May 22 2019 at 11:51, on Zulip):

maybe that's better anyway

Santiago Pastorino (May 22 2019 at 12:03, on Zulip):

@oli have changed it and force pushed https://github.com/rust-lang/rust/pull/61030

Santiago Pastorino (May 22 2019 at 12:04, on Zulip):

I don't understand why I'm getting error value assigned to 'o' is never read

Santiago Pastorino (May 22 2019 at 12:16, on Zulip):

there's a tiny error on what I did, still the error value assigned to o thing unsure

oli (May 22 2019 at 12:17, on Zulip):

probably because of the return that I added a comment on

Santiago Pastorino (May 22 2019 at 12:17, on Zulip):

ahh ya, hehe

Santiago Pastorino (May 22 2019 at 12:23, on Zulip):

@oli should be ready now

Santiago Pastorino (May 22 2019 at 12:23, on Zulip):

pushed but running tests locally also, just in case

Santiago Pastorino (May 22 2019 at 12:24, on Zulip):

as you may see the None at the end and some empty things were to avoid the else of the if let and some other return None cases and just have it at the end

Santiago Pastorino (May 22 2019 at 12:24, on Zulip):

that's I guess why the original code was in that way

Santiago Pastorino (May 22 2019 at 12:24, on Zulip):

anyway, it's more or less similar

Santiago Pastorino (May 22 2019 at 12:24, on Zulip):

:)

oli (May 22 2019 at 12:25, on Zulip):

yea, I prefer the new code you wrote, even if there are some additional elses

Santiago Pastorino (May 22 2019 at 12:27, on Zulip):

:+1:

Santiago Pastorino (May 22 2019 at 12:27, on Zulip):

tests are ok

Santiago Pastorino (May 22 2019 at 18:36, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61051 doesn't work, need to check what's going on

Santiago Pastorino (May 23 2019 at 15:03, on Zulip):

@oli pushed again https://github.com/rust-lang/rust/pull/61051, still with failures :)

Santiago Pastorino (May 23 2019 at 15:03, on Zulip):

with the Option trick we talked about

Santiago Pastorino (May 23 2019 at 19:42, on Zulip):

@oli another one https://github.com/rust-lang/rust/pull/61092

Santiago Pastorino (May 23 2019 at 19:44, on Zulip):

and https://github.com/rust-lang/rust/pull/61093

Santiago Pastorino (May 23 2019 at 20:18, on Zulip):

https://github.com/rust-lang/rust/pull/61094

Santiago Pastorino (May 23 2019 at 20:51, on Zulip):

https://github.com/rust-lang/rust/pull/61099

Santiago Pastorino (May 23 2019 at 23:10, on Zulip):

https://github.com/rust-lang/rust/pull/61102

Santiago Pastorino (May 23 2019 at 23:16, on Zulip):

that last one seems to be not ok, unsure why

Santiago Pastorino (May 23 2019 at 23:16, on Zulip):

https://github.com/rust-lang/rust/pull/61103

Santiago Pastorino (May 23 2019 at 23:29, on Zulip):

https://github.com/rust-lang/rust/pull/61104

Santiago Pastorino (May 23 2019 at 23:30, on Zulip):

this one ^^^ is also not ok

Santiago Pastorino (May 25 2019 at 20:29, on Zulip):

@RalfJ https://github.com/rust-lang/rust/pull/61193

Santiago Pastorino (May 27 2019 at 20:01, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61241

oli (May 27 2019 at 20:02, on Zulip):

already open :P

oli (May 27 2019 at 20:02, on Zulip):

irc pings are super fast

Santiago Pastorino (May 27 2019 at 20:02, on Zulip):

:P

Santiago Pastorino (May 27 2019 at 20:02, on Zulip):

do you have some kind of alert on github mentions?

Santiago Pastorino (May 27 2019 at 20:03, on Zulip):

tidy is ok and tests are ok

Santiago Pastorino (May 27 2019 at 20:03, on Zulip):

there's another one coming which I'm running all the checks :)

oli (May 27 2019 at 20:05, on Zulip):

there's an irc channel on the mozilla irc that will ping reviewers

oli (May 27 2019 at 20:05, on Zulip):

although I'm not sure what I'm gonna do when the mozilla irc is shut down

Santiago Pastorino (May 27 2019 at 20:11, on Zulip):

hehehe

Santiago Pastorino (May 27 2019 at 20:11, on Zulip):

Zulip pings? :P

oli (May 27 2019 at 20:14, on Zulip):

not enough automation

Santiago Pastorino (May 27 2019 at 20:15, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61242 (auto msg)

Santiago Pastorino (May 27 2019 at 20:15, on Zulip):

:P

oli (May 27 2019 at 20:15, on Zulip):

xD

oli (May 27 2019 at 20:15, on Zulip):

you were pretty close time-wise this time

oli (May 27 2019 at 20:15, on Zulip):

not even a second delay

Santiago Pastorino (May 27 2019 at 20:16, on Zulip):

already fixed the other PR, testing just in case

Santiago Pastorino (May 27 2019 at 20:18, on Zulip):

what's the meaning of this https://github.com/rust-lang/rust/pull/61242#issuecomment-496298345 ?

oli (May 27 2019 at 20:19, on Zulip):

you can now @bors r=oli-obk without needing me to do anything

oli (May 27 2019 at 20:19, on Zulip):

since I'm going to bed soonish

Santiago Pastorino (May 27 2019 at 20:19, on Zulip):

and this https://github.com/rust-lang/rust/pull/61241 is fixed

Santiago Pastorino (May 27 2019 at 20:19, on Zulip):

you can now @bors r=oli-obk without needing me to do anything

I was guessing that yeah :)

Santiago Pastorino (May 27 2019 at 20:19, on Zulip):

wouldn't be better to rollup?

oli (May 27 2019 at 20:21, on Zulip):

if anyone makes a rollup, they'll probably be included irrelevant of whether they are marked with rollup

Santiago Pastorino (May 27 2019 at 20:53, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61247

Santiago Pastorino (May 27 2019 at 21:04, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61248

Santiago Pastorino (May 27 2019 at 21:45, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61249

centril (May 28 2019 at 10:18, on Zulip):

@oli is correct :P

Santiago Pastorino (May 28 2019 at 11:19, on Zulip):

@centril should this one https://github.com/rust-lang/rust/pull/61249 be included?

centril (May 28 2019 at 11:19, on Zulip):

@Santiago Pastorino I was worried it would break my rollup cause you use local and rename it in two different PRs

centril (May 28 2019 at 11:20, on Zulip):

I'll include it in the next rollup

Santiago Pastorino (May 28 2019 at 11:25, on Zulip):

yeah, true :)

Santiago Pastorino (May 28 2019 at 13:22, on Zulip):

@oli https://github.com/rust-lang/rust/pull/61261

Santiago Pastorino (May 28 2019 at 15:43, on Zulip):

@centril https://github.com/rust-lang/rust/pull/61261 I guess I can also remove the middle commit

Santiago Pastorino (May 28 2019 at 15:44, on Zulip):

I don't like that much doing this kind of back and forth ... hmm

Santiago Pastorino (May 28 2019 at 15:44, on Zulip):

will be back in a bit

Santiago Pastorino (May 28 2019 at 19:40, on Zulip):

@oli @centril https://github.com/rust-lang/rust/pull/61249 and https://github.com/rust-lang/rust/pull/61261 both seems to be ready again to r+

Santiago Pastorino (May 28 2019 at 19:41, on Zulip):

ci finished

oli (May 29 2019 at 07:16, on Zulip):

I don't like that much doing this kind of back and forth ... hmm

:frown: yea, not fun. But on the other hand, the code is super cleaned up now. Do you have any suggestions how to handle this better from the start?

Santiago Pastorino (Jun 20 2019 at 15:10, on Zulip):

so ... current status of this for other to see ...

Santiago Pastorino (Jun 20 2019 at 15:11, on Zulip):
error: variable does not need to be mutable
    --> src/libcore/slice/mod.rs:3182:40
     |
3014 | / macro_rules! iterator {
3015 | |     (
3016 | |         struct $name:ident -> $ptr:ty,
3017 | |         $elem:ty,
...    |
3182 | |             fn rposition<P>(&mut self, mut predicate: P) -> Option<usize> where
     | |                                        ----^^^^^^^^^
     | |                                        |
     | |                                        help: remove this `mut`
...    |
3260 | |     }
3261 | | }
     | |_- in this expansion of `iterator!`
...
3476 |   iterator!{struct IterMut -> *mut T, &'a mut T, mut, {mut}, {}}
     |   -------------------------------------------------------------- in this macro invocation

error: aborting due to 59 previous errors
Santiago Pastorino (Jun 20 2019 at 15:11, on Zulip):

getting 59 errors like this one, variable does not need to be mutable

Santiago Pastorino (Jun 20 2019 at 15:11, on Zulip):

I'm checking again but we have checked with @oli and have no clue what's going on for now

Santiago Pastorino (Jun 20 2019 at 15:12, on Zulip):

PR is https://github.com/rust-lang/rust/pull/60913

Santiago Pastorino (Jun 20 2019 at 15:12, on Zulip):

/cc @nikomatsakis @eddyb in case you guys want to review the PR or help me debug :)

Santiago Pastorino (Jun 24 2019 at 15:48, on Zulip):

@oli just opened this https://github.com/rust-lang/rust/pull/62096

Santiago Pastorino (Jun 24 2019 at 15:49, on Zulip):

btw, just to update the Place 2.0 status here, I've found the mut error displayed in the previous messages

Santiago Pastorino (Jun 24 2019 at 15:50, on Zulip):

getting other errors that for now we couldn't fix

Santiago Pastorino (Jun 24 2019 at 15:50, on Zulip):

https://gist.github.com/spastorino/7da6225651fb17ad4f0d8f48b4b32277

Santiago Pastorino (Jun 24 2019 at 15:50, on Zulip):

in case someone wants to take a look

Santiago Pastorino (Jun 24 2019 at 17:57, on Zulip):

oli just opened this https://github.com/rust-lang/rust/pull/62096

@centril addressed your comments

centril (Jun 24 2019 at 18:00, on Zulip):

thanks

Santiago Pastorino (Jun 24 2019 at 18:05, on Zulip):

thanks to you :)

Santiago Pastorino (Jun 24 2019 at 18:05, on Zulip):

I always have doubts about using Self vs explicitly using the type there

Santiago Pastorino (Jun 24 2019 at 18:06, on Zulip):

if the block is a bit long it may be confusing to see Self but well I guess you should have a nice IDE :)

centril (Jun 24 2019 at 18:36, on Zulip):

@Santiago Pastorino My philosophy on Self is to always use it when possible

centril (Jun 24 2019 at 18:36, on Zulip):

It's very nice for maintainability

Santiago Pastorino (Jun 25 2019 at 13:48, on Zulip):

@oli @centril rebased https://github.com/rust-lang/rust/pull/62096 after the conflict, how is the right procedure to follow after the PR was r+, there's a conflict and I've already fixed it? should I do r=oli ? should somebody else do that?

centril (Jun 25 2019 at 13:50, on Zulip):

r+ed

centril (Jun 25 2019 at 13:51, on Zulip):

@Santiago Pastorino if you are doing a trivial rebase that has no impact on the idea of the PR then just r=reviewer

centril (Jun 25 2019 at 13:51, on Zulip):

same with tidy problems and such

Santiago Pastorino (Jun 25 2019 at 13:56, on Zulip):

yeah, was exactly that

Santiago Pastorino (Jun 25 2019 at 13:56, on Zulip):

zero risk

Santiago Pastorino (Jun 25 2019 at 13:56, on Zulip):

thanks

nikomatsakis (Jun 25 2019 at 16:35, on Zulip):

@Santiago Pastorino which PR contains the code that you're having problems with, if I were to go check it out? Is it #60913?

Santiago Pastorino (Jun 25 2019 at 16:35, on Zulip):

yes

Santiago Pastorino (Jun 25 2019 at 16:35, on Zulip):

that one

Santiago Pastorino (Jun 25 2019 at 16:36, on Zulip):

I've run master and place branches and compared the debug info and it's the same

Santiago Pastorino (Jun 25 2019 at 16:36, on Zulip):

was hoping to see some differences

nikomatsakis (Jun 25 2019 at 17:24, on Zulip):

@Santiago Pastorino so I get a panic like this

thread 'rustc' panicked at 'Layout mismatch when copying!
nikomatsakis (Jun 25 2019 at 17:39, on Zulip):

that looks different from what you get, right @Santiago Pastorino ?

Santiago Pastorino (Jun 25 2019 at 18:16, on Zulip):

@nikomatsakis yes

Santiago Pastorino (Jun 25 2019 at 19:25, on Zulip):

so this is where it's failing in case you haven't seen the thing https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/place.rs#L827

Santiago Pastorino (Jun 25 2019 at 19:26, on Zulip):

src.layout.details is different than dest.layout.details

Santiago Pastorino (Jun 25 2019 at 19:26, on Zulip):

src.layout.details ...

Santiago Pastorino (Jun 25 2019 at 19:26, on Zulip):
            variants: Single {
                index: 0,
            },
            fields: Array {
                stride: Size {
                    raw: 1,
                },
                count: 16,
            },
            abi: Aggregate {
                sized: true,
            },
            align: AbiAndPrefAlign {
                abi: Align {
                    pow2: 0,
                },
                pref: Align {
                    pow2: 0,
                },
            },
            size: Size {
                raw: 16,
            },
Santiago Pastorino (Jun 25 2019 at 19:26, on Zulip):

dest.layout.details ...

Santiago Pastorino (Jun 25 2019 at 19:27, on Zulip):
            variants: Single {
                index: 0,
            },
            fields: Union(
                2,
            ),
            abi: Aggregate {
                sized: true,
            },
            align: AbiAndPrefAlign {
                abi: Align {
                    pow2: 4,
                },
                pref: Align {
                    pow2: 4,
                },
            },
            size: Size {
                raw: 16,
            },
Santiago Pastorino (Jun 25 2019 at 19:27, on Zulip):

fields Array for src and fields Union for dest

Santiago Pastorino (Jun 25 2019 at 19:27, on Zulip):

unsure where is that generated exactly

oli (Jun 25 2019 at 19:40, on Zulip):

What's the concrete union's type name

oli (Jun 25 2019 at 19:40, on Zulip):

Maybe you can grep that

nikomatsakis (Jun 25 2019 at 19:56, on Zulip):

yeah ok

nikomatsakis (Jun 25 2019 at 19:56, on Zulip):

I'm also trying with -Zdump-mir=all

nikomatsakis (Jun 25 2019 at 19:56, on Zulip):

though that's...taking a lon time :)

nikomatsakis (Jun 25 2019 at 19:57, on Zulip):

seems like it dies after generating rustc.raw-imp-{{impl}}-static_empty.003-018.ConstProp.before.mir

nikomatsakis (Jun 25 2019 at 19:58, on Zulip):

OOPS

nikomatsakis (Jun 25 2019 at 20:00, on Zulip):

that seems to roughly align with

query stack during panic:
#0 [optimized_mir] processing `raw::imp::Group::static_empty`
end of query stack
nikomatsakis (Jun 25 2019 at 20:00, on Zulip):

though I'm not sure it's 100% right

nikomatsakis (Jun 25 2019 at 20:01, on Zulip):
// MIR for `raw::imp::<impl at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:23:1: 140:2>::static_empty`
// source = MirSource { instance: Item(DefId(0:1389 ~ hashbrown[b7a2]::raw[0]::imp[0]::{{impl}}[0]::static_empty[0])), promoted: None }
// pass_name = ConstProp
// disambiguator = before

fn  raw::imp::<impl at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:23:1: 140:2>::static_empty() -> &[u8] {
    let mut _0: &[u8];                   // return place in scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:32:30: 32:43
    let mut _1: &[u8; 16];               // in scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
    let mut _2: &[u8; 16];               // in scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
    let mut _3: raw::imp::Group::static_empty::AlignedBytes; // in scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:19: 40:32
    scope 1 {
    }

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
        StorageLive(_2);                 // bb0[1]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
        _2 = &(promoted[0]: raw::imp::Group::static_empty::AlignedBytes); // bb0[2]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
        _1 = _2;                         // bb0[3]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
        _0 = move _1 as &[u8] (Pointer(Unsize)); // bb0[4]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18: 40:38
        StorageDead(_1);                 // bb0[5]: scope 1 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:37: 40:38
        StorageDead(_2);                 // bb0[6]: scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:41:5: 41:6
        return;                          // bb0[7]: scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:41:6: 41:6
    }

    bb1 (cleanup): {
        resume;                          // bb1[0]: scope 0 at /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:32:5: 41:6
    }
}
nikomatsakis (Jun 25 2019 at 20:02, on Zulip):

there, that looks right

nikomatsakis (Jun 25 2019 at 20:04, on Zulip):

though I don't quite understand which copy is generating the problem

nikomatsakis (Jun 25 2019 at 20:04, on Zulip):

still, _2 = &(promoted[0]: AlignedBytes) seems wrong

nikomatsakis (Jun 25 2019 at 20:05, on Zulip):

later we get this error:

error: internal compiler error: broken MIR in DefId(0:1389 ~ hashbrown[b7a2]::raw[0]::imp[0]::{{impl}}[0]::static_empty[0]) (_0 = move (_1.1: [u8; 16])): bad assignment (raw::imp::Group::static_empty::Align\
edBytes = [u8; 16]): NoSolution
  --> /home/nmatsakis/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.4.0/src/raw/sse2.rs:40:18
   |
40 |         unsafe { &ALIGNED_BYTES.bytes }
   |                  ^^^^^^^^^^^^^^^^^^^^
nikomatsakis (Jun 25 2019 at 20:05, on Zulip):

which seems also in that same spot

nikomatsakis (Jun 25 2019 at 20:05, on Zulip):

still, _2 = &(promoted[0]: AlignedBytes) seems wrong

to be clear, it seems like a type mismatch: the RHS is &AlignedBytes, I guess, and the LHS is &[u8; 16]

nikomatsakis (Jun 25 2019 at 20:06, on Zulip):

I think the problem is just the type

nikomatsakis (Jun 25 2019 at 20:06, on Zulip):

if I look at rustc.raw-imp-{{impl}}-static_empty-promoted[0].002-001.SimplifyCfg-qualify-consts.after.mir

nikomatsakis (Jun 25 2019 at 20:06, on Zulip):

it has:

nikomatsakis (Jun 25 2019 at 20:06, on Zulip):

_0 = move (_1.1: [u8; 16]);

nikomatsakis (Jun 25 2019 at 20:07, on Zulip):

which looks correct

nikomatsakis (Jun 25 2019 at 20:07, on Zulip):

however, it also has let mut _0: raw::imp::Group::static_empty::AlignedBytes;

nikomatsakis (Jun 25 2019 at 20:07, on Zulip):

which looks wrong

nikomatsakis (Jun 25 2019 at 20:07, on Zulip):

@Santiago Pastorino so it seems like the problem lies in the updates to the "constant promotion" code

Santiago Pastorino (Jun 25 2019 at 20:08, on Zulip):

yeah, I've reached that final conclusion a bit less informed than your procedure :P, but couldn't find what's wrong there

nikomatsakis (Jun 25 2019 at 20:08, on Zulip):

src/librustc_mir/transform/promote_consts.rs or maybe src/librustc_mir/transform/qualify_consts.rs

nikomatsakis (Jun 25 2019 at 20:08, on Zulip):

although I think that @oli is the world expert on that code :)

nikomatsakis (Jun 25 2019 at 20:08, on Zulip):

well, let's see...

Santiago Pastorino (Jun 25 2019 at 20:08, on Zulip):

and if you talk about the type I guess the Place::ty_from calls are the suspicious ones

Santiago Pastorino (Jun 25 2019 at 20:09, on Zulip):

I've found in a place where I used proj when I should have used proj.base in the past

Santiago Pastorino (Jun 25 2019 at 20:09, on Zulip):

but right now that doesn't seem to be the case

nikomatsakis (Jun 25 2019 at 20:09, on Zulip):

OK -- I have to run at the moment

Santiago Pastorino (Jun 25 2019 at 20:11, on Zulip):

this section https://github.com/rust-lang/rust/pull/60913/files#diff-bfc61a84a9e3b48761db14b8d6772a0aL311 is a bit suspicious

Santiago Pastorino (Jun 25 2019 at 20:14, on Zulip):

and actually that's only place_base and None, no need to do the while thing

Santiago Pastorino (Jun 25 2019 at 20:54, on Zulip):

@oli @nikomatsakis the problem is here https://github.com/rust-lang/rust/pull/60913/files#diff-bfc61a84a9e3b48761db14b8d6772a0aL312

Santiago Pastorino (Jun 25 2019 at 20:54, on Zulip):

if instead of going to the traversing the projection and end in the base, I just use the place that's there the code compiles

Santiago Pastorino (Jun 25 2019 at 20:55, on Zulip):

of course that's not what the code was doing before, I was just playing with things to see what happens

Santiago Pastorino (Jun 25 2019 at 20:55, on Zulip):

so ... from the old code seems like I need to use just the base

Santiago Pastorino (Jun 25 2019 at 20:55, on Zulip):

one of my tries was to replace all that code with let ty = place.base.ty(local_decls).ty; but that doesn't work

Santiago Pastorino (Jun 25 2019 at 20:55, on Zulip):

unsure why and what's going on

Santiago Pastorino (Jun 25 2019 at 20:59, on Zulip):

hmm I think we need to do ty over place.base and call replace over place.base too

Santiago Pastorino (Jun 25 2019 at 20:59, on Zulip):

I'm not sure and need to go now, going to check this later/tomorrow

oli (Jun 25 2019 at 21:19, on Zulip):

:face_palm: of course

oli (Jun 25 2019 at 21:19, on Zulip):

The code below uses the place variable that used to be mutated by the old code

Santiago Pastorino (Jun 26 2019 at 03:45, on Zulip):

so yeah, definitely, the code is compiling and tests passing right now :)

Santiago Pastorino (Jun 26 2019 at 03:46, on Zulip):

@oli @nikomatsakis ^^^

Santiago Pastorino (Jun 26 2019 at 03:46, on Zulip):

@oli there are 2 new commits that I'm not sure you have checked

Santiago Pastorino (Jun 26 2019 at 03:46, on Zulip):

https://github.com/rust-lang/rust/pull/60913/commits/2422a0f368a7b575720cce3056f53562a1d8338f

Santiago Pastorino (Jun 26 2019 at 03:46, on Zulip):

https://github.com/rust-lang/rust/pull/60913/commits/73cf88b959b3836213c23792e50e61330042d49d

Santiago Pastorino (Jun 26 2019 at 03:47, on Zulip):

if you're ok with those I can ...

Santiago Pastorino (Jun 26 2019 at 03:47, on Zulip):

1. squash everything into one commit with a meaningful commit message

Santiago Pastorino (Jun 26 2019 at 03:48, on Zulip):

2. fix tidy done in https://github.com/rust-lang/rust/pull/60913/commits/542081f670a258f3d9cda6e978a8b74d9dba69ca

Santiago Pastorino (Jun 26 2019 at 03:48, on Zulip):

3. get rid of clones (?)

Santiago Pastorino (Jun 26 2019 at 03:48, on Zulip):

or do you have a different plan?

Santiago Pastorino (Jun 26 2019 at 03:55, on Zulip):

just have rebased on top of the Place::from change, so the diff should be a bit smaller

oli (Jun 26 2019 at 07:29, on Zulip):

Yes, only 3. is left now

nikomatsakis (Jun 26 2019 at 10:05, on Zulip):

so yeah, definitely, the code is compiling and tests passing right now :)

woohoo!

Santiago Pastorino (Jun 26 2019 at 11:47, on Zulip):

@oli about the clones, do you think we need to pass around place_base and place_projection separately?

Santiago Pastorino (Jun 26 2019 at 11:48, on Zulip):

should we use some kind of abstraction over that?

oli (Jun 26 2019 at 11:48, on Zulip):

hmm... an abstraction may make it easier (less churn) to move to slices

Santiago Pastorino (Jun 26 2019 at 11:48, on Zulip):

what kind of abstraction are you thinking about that make things easier to move to slices?

oli (Jun 26 2019 at 11:50, on Zulip):

If you have a struct PlaceRef<'a> { base: &'a PlaceBase, proj: Option<&'a Projection> }, then when we move to slices we can just change the proj field in the definition instead of touching every use of the PlaceRef struct

Santiago Pastorino (Jun 26 2019 at 11:52, on Zulip):

yeah, that's exactly what I was thinking about PlaceRef

oli (Jun 26 2019 at 11:54, on Zulip):

heh, we're slowly syncing our brains on place related things

Santiago Pastorino (Jun 26 2019 at 11:56, on Zulip):

yeah, to be honest wasn't seeing that clearly that would make migrating to slices easier

Santiago Pastorino (Jun 26 2019 at 11:57, on Zulip):

:)

centril (Jun 26 2019 at 12:00, on Zulip):

@Santiago Pastorino Reading your PRs makes me want https://ghc.haskell.org/trac/ghc/wiki/PatternSynonyms so much :D

Santiago Pastorino (Jun 26 2019 at 13:46, on Zulip):

@centril hehe, yeah, seems good :)

Santiago Pastorino (Jun 26 2019 at 13:47, on Zulip):

what pattern repetition in particular caused you to think that?

Santiago Pastorino (Jun 26 2019 at 13:47, on Zulip):

I know there are a bunch, just asking for curiosity :)

Santiago Pastorino (Jun 26 2019 at 13:47, on Zulip):

I guess:

Santiago Pastorino (Jun 26 2019 at 13:47, on Zulip):
Place {
    base: PlaceBase::Local(local),
    projection: None,
}
Santiago Pastorino (Jun 26 2019 at 13:47, on Zulip):

that's one :)

centril (Jun 26 2019 at 13:48, on Zulip):

oh yes

centril (Jun 26 2019 at 13:48, on Zulip):

that one

oli (Jun 26 2019 at 13:48, on Zulip):

I think we'll be able to remove a bunch of these in a cleanup PR

centril (Jun 26 2019 at 13:48, on Zulip):

LocalPlace(local) =>

Santiago Pastorino (Jun 26 2019 at 14:02, on Zulip):

I think we'll be able to remove a bunch of these in a cleanup PR

also true :)

Santiago Pastorino (Jun 26 2019 at 14:37, on Zulip):

now that this compiles and tests are green, we were discussing a bit with @oli how to fix some things that I did to make the first step easier

Santiago Pastorino (Jun 26 2019 at 14:37, on Zulip):

in particular I've used some clones that should not end there

Santiago Pastorino (Jun 26 2019 at 14:38, on Zulip):

the idea was that in this same PR I'm going to put another commit to get rid of them

Santiago Pastorino (Jun 26 2019 at 14:38, on Zulip):

for instance https://github.com/rust-lang/rust/pull/60913/files#diff-56ccd2e94681399314ad7501883bd2a5R471 we build a Place again there

Santiago Pastorino (Jun 26 2019 at 14:39, on Zulip):

we should be passing a reference to base and projection down

Santiago Pastorino (Jun 26 2019 at 14:39, on Zulip):

we were mentioning two possibilities

Santiago Pastorino (Jun 26 2019 at 14:39, on Zulip):

or we do PlaceRef<'a> { base: &'a PlaceBase, proj: Option<&'a Projection> }

Santiago Pastorino (Jun 26 2019 at 14:40, on Zulip):

or the other alternative is to keep using Place and just intern the fields

Santiago Pastorino (Jun 26 2019 at 14:40, on Zulip):

/cc @eddyb @nikomatsakis in particular but @others are welcome to give some thoughts :)

centril (Aug 14 2019 at 17:23, on Zulip):

@Santiago Pastorino the new bootstrap compiler is being changed in a PR that's being tested by bors right now :slight_smile:

Santiago Pastorino (Aug 14 2019 at 18:26, on Zulip):

@centril great, so I can get rid of a lot of weird things due to now being able to use slice patterns :)

centril (Aug 14 2019 at 18:26, on Zulip):

yea

centril (Aug 14 2019 at 18:26, on Zulip):

@Santiago Pastorino hopefully slice patterns will be stable soon

centril (Aug 15 2019 at 08:17, on Zulip):

@Santiago Pastorino good news; the new bootstrap compiler is out :slight_smile:

oli (Aug 15 2019 at 08:32, on Zulip):

Yay

Santiago Pastorino (Aug 25 2019 at 02:08, on Zulip):

@centril @oli fixes for Centril comments https://github.com/rust-lang/rust/pull/63874

centril (Aug 25 2019 at 02:09, on Zulip):

@Santiago Pastorino reviewed

Santiago Pastorino (Aug 25 2019 at 02:10, on Zulip):

cool

Santiago Pastorino (Aug 25 2019 at 02:11, on Zulip):

btw, I've just pushed the main one

Santiago Pastorino (Aug 25 2019 at 02:11, on Zulip):

https://github.com/rust-lang/rust/pull/63420

Santiago Pastorino (Aug 25 2019 at 02:11, on Zulip):

@oli we may want to run perf again I guess

Santiago Pastorino (Aug 25 2019 at 02:11, on Zulip):

but max-rss is very bad in some cases

Santiago Pastorino (Aug 25 2019 at 02:11, on Zulip):

I'd leave the judge up to you but we can wait the merge and keep working on interning the thing in the same PR, new commits

oli (Aug 25 2019 at 02:21, on Zulip):

Heh kekkac obviously

oli (Aug 25 2019 at 02:21, on Zulip):

That's like the mother of all mir Bodies

oli (Aug 25 2019 at 02:31, on Zulip):

Maybe we should just use smallvec<Elem> instead of Box<[Elem]>

oli (Aug 25 2019 at 02:31, on Zulip):

Not sure if that helps

Santiago Pastorino (Aug 25 2019 at 02:35, on Zulip):

hehehe

Santiago Pastorino (Aug 25 2019 at 02:35, on Zulip):

just force pushed again, there was a tidy error

Santiago Pastorino (Aug 25 2019 at 02:37, on Zulip):

ran perf again, things are not going to change much I guess

Santiago Pastorino (Aug 25 2019 at 02:37, on Zulip):

didn't try with smallvec

Santiago Pastorino (Aug 25 2019 at 02:37, on Zulip):

do you think it worth? or should we go directly to interning?

oli (Aug 25 2019 at 03:12, on Zulip):

Yea we could just intern directly. Keep that in a different commit tho

Wesley Wiser (Aug 26 2019 at 14:56, on Zulip):

@Santiago Pastorino if there's anything I can help with, please let me know! I'd love to get this merged :slight_smile:

Santiago Pastorino (Aug 26 2019 at 15:03, on Zulip):

@Wesley Wiser probably not for now, but I guess we can talk and I'm 100% open to pair on this if you want

Santiago Pastorino (Aug 26 2019 at 15:04, on Zulip):

I think right now things are not very parallelizable

Wesley Wiser (Aug 26 2019 at 15:04, on Zulip):

Well, I don't want to slow you down

Wesley Wiser (Aug 26 2019 at 15:04, on Zulip):

But if you hit a point where you could hand something off, feel free to ping me

Santiago Pastorino (Aug 26 2019 at 15:04, on Zulip):

sure

Santiago Pastorino (Aug 26 2019 at 15:05, on Zulip):

also, first I may need to finish something that it's 95% done and then we can talk I guess

Wesley Wiser (Aug 26 2019 at 15:05, on Zulip):

Sounds good!

ecstatic-morse (Sep 04 2019 at 01:12, on Zulip):

@Santiago Pastorino, when you get a chance could you take a look at #64005?

Santiago Pastorino (Sep 04 2019 at 04:24, on Zulip):

yes, gonna try to do it tomorrow

Santiago Pastorino (Sep 04 2019 at 04:24, on Zulip):

sorry about that, was very busy after RustConf

ecstatic-morse (Sep 04 2019 at 04:25, on Zulip):

np

Santiago Pastorino (Sep 04 2019 at 04:29, on Zulip):

@ecstatic-morse actually, just did it :)

ecstatic-morse (Sep 04 2019 at 04:38, on Zulip):

Thanks! Yeah the name is a little awkward, especially since using it results in a lot of !is_indirect(). is_direct is too opaque, and I think has_deref_projectionis maybe too literal? I originally had a method called base_direct which returned Some(base) if there were no deref projections.

ecstatic-morse (Sep 04 2019 at 04:39, on Zulip):

but now that you don't need to traverse projections to get to the base, that seems a bit silly

ecstatic-morse (Sep 04 2019 at 04:41, on Zulip):

@Santiago Pastorino ^

ecstatic-morse (Sep 04 2019 at 05:21, on Zulip):

I kind of like contains_indirection or contains_deref. is_indirection makes it seem like it only returns true for *x, not (*x.field)[2]. is_indirect is nice because it's short and more abstract.

Santiago Pastorino (Sep 13 2019 at 19:39, on Zulip):

this other bit https://github.com/rust-lang/rust/pull/63420 is in

Santiago Pastorino (Sep 13 2019 at 19:40, on Zulip):

@Wesley Wiser what are you currently working on? :)

Wesley Wiser (Sep 13 2019 at 19:41, on Zulip):

@Santiago Pastorino https://github.com/rust-lang/rust/pull/64419

Wesley Wiser (Sep 13 2019 at 19:41, on Zulip):

I think it will be ready to merge sometime this weekend.

Santiago Pastorino (Sep 13 2019 at 19:43, on Zulip):

cool

Wesley Wiser (Sep 13 2019 at 19:44, on Zulip):

What's the next step for Places?

Santiago Pastorino (Sep 13 2019 at 19:45, on Zulip):

my pr is going to give you some pains :P

Santiago Pastorino (Sep 13 2019 at 19:45, on Zulip):

well, not that many I think

Santiago Pastorino (Sep 13 2019 at 19:45, on Zulip):

this https://github.com/rust-lang/rust/pull/64419/files#diff-9e103702275cbef342c2decd3395bf3bL285 doesn't exist anymore

Santiago Pastorino (Sep 13 2019 at 19:46, on Zulip):

What's the next step for Places?

there are not big plans yet afaik

Santiago Pastorino (Sep 13 2019 at 19:46, on Zulip):

gonna do little things meanwhile

Wesley Wiser (Sep 13 2019 at 19:46, on Zulip):

I actually got rid of that call entirely so that should be easy to merge.

Wesley Wiser (Sep 13 2019 at 19:46, on Zulip):

Gotcha

Wesley Wiser (Sep 13 2019 at 19:50, on Zulip):

I might play around with this https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/Place.20size/near/171701734 unless that's one of the things you were going to look at

Santiago Pastorino (Sep 13 2019 at 20:13, on Zulip):

@Wesley Wiser that would have been the big next step

Santiago Pastorino (Sep 13 2019 at 20:13, on Zulip):

but we have talked about it with Oli and he said that's better to put that on hold until there's a design meeting

Santiago Pastorino (Sep 13 2019 at 20:14, on Zulip):

I guess once we do that we could talk about it?

Santiago Pastorino (Sep 13 2019 at 20:15, on Zulip):

I want to do that as next step yeah

Santiago Pastorino (Sep 16 2019 at 19:46, on Zulip):

@centril https://github.com/rust-lang/rust/pull/64518#discussion_r324852427 what idiom exactly are you referring to?

centril (Sep 16 2019 at 19:48, on Zulip):

@Santiago Pastorino

                let mut cursor = &**projection;
                while let [proj_base @ .., elem] = cursor {
                    cursor = proj_base;

==>

projection.stuff(|cursor, proj_base, elem| {
});

or some such

centril (Sep 16 2019 at 19:51, on Zulip):

or just projection.stuff(|proj_base, elem| { ... }) since cursor isn't used in the loops

Santiago Pastorino (Sep 16 2019 at 19:51, on Zulip):

what we want is ...

Santiago Pastorino (Sep 16 2019 at 19:51, on Zulip):

for (proj_base, elem) in place.projection.iter() :)

centril (Sep 16 2019 at 19:52, on Zulip):

yea ok

Santiago Pastorino (Sep 16 2019 at 19:52, on Zulip):

the problem is that this is a slice

Santiago Pastorino (Sep 16 2019 at 19:53, on Zulip):

so we may need to go and do ...

Santiago Pastorino (Sep 16 2019 at 19:54, on Zulip):

for (proj_base, elem) in place.proj_iter()

Santiago Pastorino (Sep 16 2019 at 19:54, on Zulip):

or something like that

Santiago Pastorino (Sep 16 2019 at 19:54, on Zulip):

to be honest unsure if worth all this

Santiago Pastorino (Sep 16 2019 at 19:54, on Zulip):

but happy to do what you guys like

Santiago Pastorino (Sep 16 2019 at 19:54, on Zulip):

/cc @oli

centril (Sep 16 2019 at 19:54, on Zulip):

@Santiago Pastorino well the only difference there is that I've baked the for-loop into .stuff

centril (Sep 16 2019 at 19:55, on Zulip):

(cause I usually like to bake in imperative control flow and restrict it... /functional-programmer)

Santiago Pastorino (Sep 16 2019 at 19:55, on Zulip):

yeah, I've seen that already from your comments on PRs :smile:

Santiago Pastorino (Sep 16 2019 at 19:55, on Zulip):

it's fine

Santiago Pastorino (Sep 16 2019 at 19:55, on Zulip):

but I kind of disagree :)

Santiago Pastorino (Sep 16 2019 at 19:56, on Zulip):

I guess is more correct to define an iterator that allows you to iterate over elem but carrying at the same time the previous elements

Santiago Pastorino (Sep 16 2019 at 19:57, on Zulip):

but I kind of disagree :)

I mean, on a given point more or less I don't care that much, but if you both want me to do that, happy to change the PR

Santiago Pastorino (Sep 16 2019 at 19:57, on Zulip):

I didn't want to have the previous things where we were carrying that index which is a bit error-prone

centril (Sep 16 2019 at 19:58, on Zulip):

I mean, on a given point more or less I don't care that much, but if you both want me to do that, happy to change the PR

It's fine; I mostly wanted less duplication but it seems you'll be fixing that in another way later

Last update: Nov 17 2019 at 07:50UTC