Stream: t-compiler/wg-mir-opt

Topic: interning next steps


nikomatsakis (Oct 01 2019 at 19:35, on Zulip):

So @Santiago Pastorino I was thinking that, after #64922, the next step for interning is probably modifying the MutVisitor so that they don't visit the contents of places

nikomatsakis (Oct 01 2019 at 19:36, on Zulip):

well, all visitors I guess

nikomatsakis (Oct 01 2019 at 19:36, on Zulip):

since once we intern we're going to have to visit a place as an atomic thing

nikomatsakis (Oct 01 2019 at 19:36, on Zulip):

I'm not sure how big a change this will be

Santiago Pastorino (Oct 01 2019 at 19:43, on Zulip):

@nikomatsakis I was starting to add the methods we talked about yesterday

Santiago Pastorino (Oct 01 2019 at 19:43, on Zulip):

do you think I should stop that and switch to this?

Santiago Pastorino (Oct 01 2019 at 19:44, on Zulip):

since once we intern we're going to have to visit a place as an atomic thing

unsure why is this, we are interning only the projections of a place first, right?

Santiago Pastorino (Oct 01 2019 at 19:44, on Zulip):

if we are interning projections, why do we end visiting a place as an atomic thing?

Santiago Pastorino (Oct 01 2019 at 19:45, on Zulip):

So Santiago Pastorino I was thinking that, after #64922, the next step for interning is probably modifying the MutVisitor so that they don't visit the contents of places

If you don't visit contents of places what do you do on each visitor? or how do you emulate what visitors are right now doing?

Santiago Pastorino (Oct 01 2019 at 19:45, on Zulip):

trying to understand exactly what I need to accomplish

nikomatsakis (Oct 01 2019 at 19:53, on Zulip):

I was starting to add the methods we talked about yesterday

which methods, @Santiago Pastorino ?

nikomatsakis (Oct 01 2019 at 19:54, on Zulip):

if we are interning projections, why do we end visiting a place as an atomic thing?

ok well we can visit the "base"

nikomatsakis (Oct 01 2019 at 19:54, on Zulip):

but we can't mut-visit the projections

nikomatsakis (Oct 01 2019 at 19:54, on Zulip):

because they are interned

nikomatsakis (Oct 01 2019 at 19:54, on Zulip):

If you don't visit contents of places what do you do on each visitor? or how do you emulate what visitors are right now doing?

I guess we have to look at the visitors and see what they do

nikomatsakis (Oct 01 2019 at 19:54, on Zulip):

for the non-mut visitors, we could have an auxiliary "place visitor" trait

nikomatsakis (Oct 01 2019 at 19:55, on Zulip):

that visits the base/projections as today

nikomatsakis (Oct 01 2019 at 19:55, on Zulip):

but is always read-only

nikomatsakis (Oct 01 2019 at 19:55, on Zulip):

for the mut visitors, well, we'll have to look carefully, they may need to do a "map" where they produce a new place and then store the whole thing back

Santiago Pastorino (Oct 01 2019 at 19:56, on Zulip):

I was starting to add the methods we talked about yesterday

which methods, Santiago Pastorino ?

I meant, I started to play with the interning process, adding these _intern_predicate like methods

Santiago Pastorino (Oct 01 2019 at 19:59, on Zulip):

ok I got that we can't mut-visit the projections because they are interned

Santiago Pastorino (Oct 01 2019 at 19:59, on Zulip):

the place visitor I guess makes sense

Santiago Pastorino (Oct 01 2019 at 20:01, on Zulip):

for the mut visitors, well, we'll have to look carefully, they may need to do a "map" where they produce a new place and then store the whole thing back

I wonder then how do you solve this, I guess all you're saying is that we can't just implement the mut visitor because we have an interned structure but we need to mutate that in some different way

Santiago Pastorino (Oct 01 2019 at 20:01, on Zulip):

I guess I'd need to take a look around to see what's exactly the issue

Santiago Pastorino (Oct 01 2019 at 20:01, on Zulip):

so my main question about this is ...

Santiago Pastorino (Oct 01 2019 at 20:02, on Zulip):

are you proposing that I should fix the mut visitors issue before hand? like to leave the code prepared for the interning case?

Santiago Pastorino (Oct 01 2019 at 20:03, on Zulip):

or should I do all at once and fix the visitors once I see the compiler error I'm getting when using List<PlaceElem<'tcx>>?

nikomatsakis (Oct 01 2019 at 20:11, on Zulip):

I meant, I started to play with the interning process, adding these _intern_predicate like methods

I see. I think that's a bit premature, we should fix the visitor things first.

nikomatsakis (Oct 01 2019 at 20:11, on Zulip):

are you proposing that I should fix the mut visitors issue before hand? like to leave the code prepared for the interning case?

yep

Santiago Pastorino (Oct 01 2019 at 20:11, on Zulip):

cool

nikomatsakis (Oct 01 2019 at 20:11, on Zulip):

I think you'd be happier trying to debug the visitor changes in isolation :)

Santiago Pastorino (Oct 01 2019 at 20:11, on Zulip):

yeah it makes sense

Santiago Pastorino (Oct 01 2019 at 20:12, on Zulip):

in general in the mut version, I guess you get the projection from the arena, modify it locally and reintern it?

Santiago Pastorino (Oct 01 2019 at 20:12, on Zulip):

I mean, I guess you don't mut visit the projection but you still will need to modify it

Santiago Pastorino (Oct 01 2019 at 20:13, on Zulip):

talking about interning, I know that I shouldn't do it for now but I still don't know to what I should change the visitors because I guess I don't understand the problem that the interning process is going to give me

Wesley Wiser (Oct 01 2019 at 20:15, on Zulip):

So right now, visit_place has a &mut Place and since Place owns the Box, visit_place() is free to mutate it

Wesley Wiser (Oct 01 2019 at 20:15, on Zulip):

Once we start interning, Place will only have a &'tcx Box<[PlaceElem]> or whatever

Wesley Wiser (Oct 01 2019 at 20:16, on Zulip):

which means the code can no longer mutate the projections

Wesley Wiser (Oct 01 2019 at 20:17, on Zulip):

It can however mutate the Place so whatever it is currently doing needs to be changed from mutating the projections in the Place directly to generating a new Box<[PlaceElem]> that we can intern and then assign to the &mut Place we have

Wesley Wiser (Oct 01 2019 at 20:18, on Zulip):

(if I understand correctly)

Wesley Wiser (Oct 01 2019 at 20:49, on Zulip):

(cc @nikomatsakis in case I'm totally wrong)

Santiago Pastorino (Oct 01 2019 at 20:52, on Zulip):

that seems reasonable

nikomatsakis (Oct 02 2019 at 00:07, on Zulip):

yep, that's it

Santiago Pastorino (Oct 02 2019 at 18:41, on Zulip):

@nikomatsakis I'm making some progress with this task but I was wondering ...

Santiago Pastorino (Oct 02 2019 at 18:41, on Zulip):

one easy way to solve this is to make visit_projection return a projection

Santiago Pastorino (Oct 02 2019 at 18:41, on Zulip):

and so the rest of the visit_* methods that visit_projection use

Santiago Pastorino (Oct 02 2019 at 18:42, on Zulip):

but that doesn't seem like a great design and even less something that a visitor would do :)

Santiago Pastorino (Oct 02 2019 at 18:43, on Zulip):

the other possibility is to have everything solved on visit_place

Santiago Pastorino (Oct 02 2019 at 18:43, on Zulip):

but still there are visit_ty and visit_local that mutates the thing and we would need those to return a new ty or new local and not to mutate

Santiago Pastorino (Oct 02 2019 at 18:45, on Zulip):

and also, is not a thing of just changing visit_ty to make it return because that one is used in other visitor methods

Santiago Pastorino (Oct 02 2019 at 18:46, on Zulip):

it's more like we need a read only version of those

Santiago Pastorino (Oct 02 2019 at 18:46, on Zulip):

I can provide a solution to this that works still wonder how to properly fit things in the current design

nikomatsakis (Oct 03 2019 at 09:05, on Zulip):

@Santiago Pastorino yeah so I think we have to be careful what we are talking about here. I'm feeling a bit confused. Today's design has a split:

Right now, these two visitors always visit the same set of things, but in one case you get to make "in place" changes. I think this is overall a nice pattern that we should probably keep, but indeed there is another alternative: you can have what we often call a "folder", where the basic signature is fn fold(&T) -> T. But this has the problem that "read-only" visits are really wasteful now, so it's often paired with a visitor fn visit(&T). It's also not great for making small edits. I rather prefer the existing mut visitor, though it might be a good idea to survey the uses and see how often we're really using it.

This is why I am advocating for moving the "visit the parts of a place" out from the MIR visitor to a separate visitor. So then you can decide how you want to visit those parts, and it can use a distinct strategy -- probably it would just use the TypeFolder methods, actually, since I think the only thing we ever want to visit inside of places is types.

nikomatsakis (Oct 03 2019 at 09:05, on Zulip):

If you want @Santiago Pastorino maybe we can schedule a bit of time to talk over this sync?

Santiago Pastorino (Oct 03 2019 at 14:28, on Zulip):

@nikomatsakis yeah, please let's sync about this for a bit

Santiago Pastorino (Oct 03 2019 at 20:21, on Zulip):

@nikomatsakis so I understand what you mean at high level

Santiago Pastorino (Oct 03 2019 at 20:22, on Zulip):

when you say move "visit the parts of a place" out from the MIR visitor to a separate visitor

Santiago Pastorino (Oct 03 2019 at 20:22, on Zulip):

I also get that we would need to move out from there things that are going to be interned because MutVisitor won't play nice with that

Santiago Pastorino (Oct 03 2019 at 20:23, on Zulip):

the thing I guess now is how would be MIR Visitor work then?

Santiago Pastorino (Oct 03 2019 at 20:23, on Zulip):

it will call that other visitor to visit place?

Santiago Pastorino (Oct 03 2019 at 20:25, on Zulip):

and I'm also understanding that that other visitor would need to take a read only place or place part or more specifically for this case would be a read only projection and return a new one by using TypeFolder methods

Santiago Pastorino (Oct 03 2019 at 20:25, on Zulip):

just wonder how the pieces fit together

oli (Oct 04 2019 at 08:54, on Zulip):

The mut visitor can also have fn visit_place_projection(&mut &'tcx [PlaceElem<'tcx>]), so you can mutate, but you have to put back a &'tcx [PlaceElem<'tcx>] in one go

nikomatsakis (Oct 04 2019 at 12:29, on Zulip):

it could, but I would avoid that

nikomatsakis (Oct 04 2019 at 12:29, on Zulip):

in particular, if we move towards interning an entire place

nikomatsakis (Oct 04 2019 at 12:29, on Zulip):

it'll just mean more work

nikomatsakis (Oct 04 2019 at 12:30, on Zulip):

I think it's easier to consider places a "leaf" type for the visitor, much like types

nikomatsakis (Oct 04 2019 at 12:30, on Zulip):

but I don't have a strong opinion on that

nikomatsakis (Oct 04 2019 at 12:30, on Zulip):

the more important thing would be to go through the visitors and see how they use the visit place methods

nikomatsakis (Oct 04 2019 at 12:36, on Zulip):

I get

/home/nmatsakis/.cargo/bin/rg --no-heading --color never 'fn visit_place\(|fn visit_place_base\(|fn visit_projection\('
librustc_codegen_ssa/mir/analyze.rs:245:    fn visit_place(&mut self,
librustc_mir/transform/inline.rs:677:    fn visit_place(&mut self,
librustc_mir/transform/qualify_consts.rs:1094:    fn visit_place_base(
librustc_mir/transform/qualify_consts.rs:1159:    fn visit_projection(
librustc_mir/monomorphize/collector.rs:659:    fn visit_place_base(&mut self,
librustc_mir/transform/generator.rs:103:    fn visit_place(&mut self,
librustc_mir/transform/generator.rs:130:    fn visit_place(&mut self,
librustc_mir/transform/generator.rs:250:    fn visit_place(&mut self,
librustc_mir/transform/check_unsafety.rs:199:    fn visit_place(&mut self,
librustc/mir/visit.rs:147:            fn visit_place(&mut self,
librustc/mir/visit.rs:154:            fn visit_place_base(&mut self,
librustc/mir/visit.rs:161:            fn visit_projection(&mut self,
librustc_mir/transform/check_consts/validation.rs:322:    fn visit_place_base(
librustc_mir/transform/check_consts/validation.rs:407:    fn visit_projection(
librustc_mir/borrow_check/nll/type_check/mod.rs:273:    fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) {
librustc_mir/borrow_check/nll/type_check/liveness/polonius.rs:68:    fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) {
nikomatsakis (Oct 04 2019 at 12:36, on Zulip):

er that's the wrong list

nikomatsakis (Oct 04 2019 at 12:36, on Zulip):

but it's a start

nikomatsakis (Oct 04 2019 at 12:37, on Zulip):

I guess what we most want to check are the MutVisitor cases

Santiago Pastorino (Oct 04 2019 at 14:43, on Zulip):

@nikomatsakis I think the main doubt I have is how would the MIR Visitor interact with that place visitor you're suggesting?

Santiago Pastorino (Oct 04 2019 at 14:44, on Zulip):

I understand that we want Place to be a leaf

Santiago Pastorino (Oct 04 2019 at 14:44, on Zulip):

that's clear

Santiago Pastorino (Oct 04 2019 at 14:46, on Zulip):

but once the MIR visitor do visit_place, I understand that we want to use the TypeFolder methods in particular for projections

nikomatsakis (Oct 04 2019 at 17:51, on Zulip):

the MIR visitor would do nothing

nikomatsakis (Oct 04 2019 at 17:52, on Zulip):

individual MIR visitors might use a type folder or whatever

nikomatsakis (Oct 04 2019 at 17:52, on Zulip):

that is basically what we do for Ty<'tcx> too, right?

nikomatsakis (Oct 04 2019 at 17:52, on Zulip):

the MIR visitor would do nothing

that is, the MIR visitor trait

nikomatsakis (Oct 04 2019 at 17:53, on Zulip):

@Santiago Pastorino :point_up:

Santiago Pastorino (Oct 04 2019 at 17:58, on Zulip):

yeah I guess we are talking or thinking about the same idea

Santiago Pastorino (Oct 04 2019 at 17:58, on Zulip):

going to code this and let you know

Santiago Pastorino (Oct 04 2019 at 17:59, on Zulip):

when I was saying what would the visitor do I was mainly referring to the default impl, anyway, let me figure out something

nikomatsakis (Oct 07 2019 at 15:07, on Zulip):

I think @Santiago Pastorino we should probably spend a bit of time auditing the visitors

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

yeah, I was taking a look at them

nikomatsakis (Oct 07 2019 at 15:07, on Zulip):

although I guess it might be useful to kind of convert them one at a time and run tests

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

yep

nikomatsakis (Oct 07 2019 at 15:07, on Zulip):

( I guess you could e.g. just add a visit_place that does nothing to do that )

Santiago Pastorino (Oct 07 2019 at 15:08, on Zulip):

my main question was about how to design reuse on this existing design

Santiago Pastorino (Oct 07 2019 at 15:09, on Zulip):

basically the idea is that we have a visit_place, visit_place_base and visit_projection all of them with default impls

Santiago Pastorino (Oct 07 2019 at 15:09, on Zulip):

what they end doing is traversing all the different parts of the place and end calling visit_ty and visit_local

Santiago Pastorino (Oct 07 2019 at 15:10, on Zulip):

so depending on the visitor some just implement visit_ty and/or visit_local and there are some that roll their own visit_place thing or visit_projection thing

nikomatsakis (Oct 07 2019 at 15:10, on Zulip):

yes

nikomatsakis (Oct 07 2019 at 15:11, on Zulip):

most of them already have some "core" thing

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

my original question was how to keep a design like this when we want at least, visit_projection, visit_ty and visit_local only for the case of projections to return new

nikomatsakis (Oct 07 2019 at 15:11, on Zulip):

e.g., visit_substs and visit_ty

nikomatsakis (Oct 07 2019 at 15:11, on Zulip):

are both "leaf" methods for the visitor

nikomatsakis (Oct 07 2019 at 15:11, on Zulip):

that have types embedded in them

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

because visit_ty and visit_local are used from other parts of the visitor

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

I think we should just go through the visitors 1 by 1

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

most of them don't even look at types, I dont' think

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

easier to look at specifics than to discuss in the abstract

Santiago Pastorino (Oct 07 2019 at 15:12, on Zulip):

yes

Santiago Pastorino (Oct 07 2019 at 15:12, on Zulip):

that's what I was also saying :)

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

ok

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

I guess the question is when to do that :)

nikomatsakis (Oct 07 2019 at 15:12, on Zulip):

I could maybe do that later today if you wanted

nikomatsakis (Oct 07 2019 at 15:13, on Zulip):

Does Place implement "type foldable" btw?

nikomatsakis (Oct 07 2019 at 15:13, on Zulip):

It does, I think, right?

Santiago Pastorino (Oct 07 2019 at 15:13, on Zulip):

yes

nikomatsakis (Oct 07 2019 at 15:13, on Zulip):

OK

nikomatsakis (Oct 07 2019 at 15:13, on Zulip):

actually

Santiago Pastorino (Oct 07 2019 at 15:13, on Zulip):

I think I can provide a PR with an idea

nikomatsakis (Oct 07 2019 at 15:13, on Zulip):

if you want I could do that right now :)

Santiago Pastorino (Oct 07 2019 at 15:14, on Zulip):

I mean, your help will always help me :) but I don't want to make you waste your time :)

Santiago Pastorino (Oct 07 2019 at 15:14, on Zulip):

what if I just start and provide a PR with some code

Santiago Pastorino (Oct 07 2019 at 15:14, on Zulip):

will do something not great from the design if needed to discuss

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

and we can make progress from there

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

ok, that works

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

if you want we can aim for some sync time later in case needed (if you have time)

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

hoping that is not needed ;)

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

I think we can basically "convert" all the existing visitors such that

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

each of them implements visit_place in a way that does not recurse into the visitor

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

then remove visit_place from the visitor

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

yeah that was my goal

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

ok

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

I guess would led to a lot of code duplication

nikomatsakis (Oct 07 2019 at 15:15, on Zulip):

well I'll be around today if you want to ping with specific questions about an individual visitor

nikomatsakis (Oct 07 2019 at 15:16, on Zulip):

I guess would led to a lot of code duplication

maybe, but I bet less than you might think

Santiago Pastorino (Oct 07 2019 at 15:16, on Zulip):

I guess would led to a lot of code duplication

but this is what I'm hoping to see from real code and stop trying to pre guess things

Santiago Pastorino (Oct 07 2019 at 15:16, on Zulip):

ok, :+1:, gonna jump into this right now

Santiago Pastorino (Oct 07 2019 at 19:41, on Zulip):

@nikomatsakis was trying things out and seeing which MutVisitors make use of visit_place in a direct or indirect way

Santiago Pastorino (Oct 07 2019 at 19:41, on Zulip):
[santiago@galago rust1 (place-mut-visitor-adjusts2)]$ rg -l "impl.*MutVisitor" src/librustc_mir/
src/librustc_mir/transform/no_landing_pads.rs
src/librustc_mir/transform/promote_consts.rs
src/librustc_mir/transform/erase_regions.rs
src/librustc_mir/transform/generator.rs
src/librustc_mir/transform/instcombine.rs
src/librustc_mir/transform/copy_prop.rs
src/librustc_mir/transform/cleanup_post_borrowck.rs
src/librustc_mir/transform/simplify.rs
src/librustc_mir/util/def_use.rs
src/librustc_mir/transform/inline.rs
src/librustc_mir/transform/const_prop.rs
src/librustc_mir/borrow_check/nll/renumber.rs
Santiago Pastorino (Oct 07 2019 at 19:42, on Zulip):

no_landing_pads seems to be the only one not used

Santiago Pastorino (Oct 07 2019 at 19:43, on Zulip):

going over them

Santiago Pastorino (Oct 07 2019 at 19:43, on Zulip):

just for the sake of discussing about something concrete

Santiago Pastorino (Oct 07 2019 at 19:43, on Zulip):

promote_consts -> https://github.com/rust-lang/rust/compare/master...spastorino:place-mut-visitor-adjusts2

Santiago Pastorino (Oct 07 2019 at 19:43, on Zulip):

it's not a lot of duplication

Santiago Pastorino (Oct 07 2019 at 19:43, on Zulip):

just duplicating https://github.com/rust-lang/rust/compare/master...spastorino:place-mut-visitor-adjusts2#diff-bfc61a84a9e3b48761db14b8d6772a0aL395

Santiago Pastorino (Oct 07 2019 at 19:44, on Zulip):

and well yeah, the visit place_base and iterate over projections

Santiago Pastorino (Oct 07 2019 at 19:45, on Zulip):

basically all the stuff that's going to be repeated is we would need to visit place_base, iterate over projections and depending on what the MutVisitor does may need to repeat some of that parts too

Santiago Pastorino (Oct 07 2019 at 19:45, on Zulip):

in particular visit_ty and/or visit_local

Santiago Pastorino (Oct 07 2019 at 19:46, on Zulip):

if implemented because the default impl is empty, what's why in this case there's nothing related to visit_ty

Santiago Pastorino (Oct 07 2019 at 19:46, on Zulip):

promote_consts -> https://github.com/rust-lang/rust/compare/master...spastorino:place-mut-visitor-adjusts2

is that more or less what you were aiming to?, I'm assuming (for now) that projection is immutable but the rest is

Santiago Pastorino (Oct 07 2019 at 19:46, on Zulip):

otherwise I can't visit_place_base

Santiago Pastorino (Oct 07 2019 at 19:46, on Zulip):

but I guess it's a first sane step

Santiago Pastorino (Oct 07 2019 at 19:53, on Zulip):

in this particular case the repetition can be easily avoided by provide a function does that the logic

nikomatsakis (Oct 07 2019 at 20:19, on Zulip):

hmm @Santiago Pastorino yeah that diff looks ok but I agree it's a bit unfortunate; I had forgotten that index places have other places embedded within

nikomatsakis (Oct 07 2019 at 20:19, on Zulip):

I guess the question is how often this will arise; I could certainly imagine having e.g. a MutPlaceVisitor or something that can extract some of that boilerplate

Santiago Pastorino (Oct 07 2019 at 20:21, on Zulip):

@nikomatsakis yeah, I guess I can continue pushing things there and following this way and see how looks like at the end or midroad

Santiago Pastorino (Oct 07 2019 at 20:22, on Zulip):

this https://github.com/rust-lang/rust/compare/master...spastorino:place-mut-visitor-adjusts2 may be a bit better :joy:

Santiago Pastorino (Oct 07 2019 at 20:24, on Zulip):

hmm Santiago Pastorino yeah that diff looks ok but I agree it's a bit unfortunate; I had forgotten that index places have other places embedded within

btw, what did you mean exactly here?, unsure if we think it is unfortunate for the same reasons

Santiago Pastorino (Oct 08 2019 at 03:45, on Zulip):

@nikomatsakis @oli https://github.com/rust-lang/rust/pull/65197

Santiago Pastorino (Oct 08 2019 at 03:46, on Zulip):

have gone over all the MutVisitors and changed the ones that needed to be changed, there's some code repetition we can discussing how to make it look better :)

Santiago Pastorino (Oct 08 2019 at 03:47, on Zulip):

I guess the default visit_place for MutVisitor could look like ...

Santiago Pastorino (Oct 08 2019 at 17:49, on Zulip):

to complete a bit more this discussion, there's the PR

Santiago Pastorino (Oct 08 2019 at 17:49, on Zulip):

we need to get rid of some duplication

Santiago Pastorino (Oct 08 2019 at 17:50, on Zulip):

I've two extra commits on a new branch that try to setup an idea we were discussing with @oli

Santiago Pastorino (Oct 08 2019 at 18:49, on Zulip):

@nikomatsakis for when you read all this stuff

Santiago Pastorino (Oct 08 2019 at 18:49, on Zulip):

I've pushed everything I have for now https://github.com/rust-lang/rust/pull/65197

Santiago Pastorino (Oct 08 2019 at 18:50, on Zulip):

the not so great things:

Santiago Pastorino (Oct 08 2019 at 18:51, on Zulip):
  1. https://github.com/rust-lang/rust/pull/65197/files#diff-f5c3f3fddb41e941652e84ee86bda82aR785-R892 there's this macro, which is not bad per se, the thing is Visitor and MutVisitor start to diverge and also the process_projection* fns doesn't seem to belong really in a visitor
Santiago Pastorino (Oct 08 2019 at 18:53, on Zulip):
  1. there are a bunch of clones like https://github.com/rust-lang/rust/pull/65197/files#diff-cbe990e8f20536f68f97230946f44f79R46 but basically all the clones there. We need to clone if we mutate something, but if the projection stays the same we shouldn't. I was talking with @oli about this, we can probably use CoW to avoid that but until we don't intern there's going to be some sort of problem like that. Still I can keep this PR going and do the interning stuff in this one so we don't land regressions.
Santiago Pastorino (Oct 08 2019 at 18:54, on Zulip):

those are the two things that I'd like to discuss a bit, about point 1. how to make the design better and about point 2. how to get rid of unneeded clones

Santiago Pastorino (Oct 09 2019 at 03:02, on Zulip):

fixed 2. so the PR here is ready, the only concern is 1. more or less the design of the solution but is ready to be reviewed and discussed /cc @nikomatsakis @oli

Last update: Nov 17 2019 at 07:25UTC