Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change EraseRegions pass to use MutVisitor #31324

Merged
merged 1 commit into from
Feb 8, 2016

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jan 31, 2016

Having a MirPass provides literally no benefits over MutVisitor. Moreover using MirPass for
EraseRegions basically makes the programmer to fix breakage from changing repr twice – in the
visitor and eraseregions. Since MutVisitor implements all the “walking” inside the trait, that can
be reused for EraseRegions too, basically resulting in less code duplication.

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@Aatch
Copy link
Contributor

Aatch commented Feb 1, 2016

Hmm, I'm all for reducing maintenance and duplication, but I worry about future development. If we want to add pass-specific functionality, this change would make that harder. Either that or require adding unrelated pass-specific methods to the visitors, which doesn't seem like a good thing. The solution to "EraseRegions duplicates visitor functionality" isn't "make all passes visitors", it's "make EraseRegions use a visitor".

/cc @rust-lang/compiler am I over-engineering this?

@nagisa
Copy link
Member Author

nagisa commented Feb 1, 2016

The solution to "EraseRegions duplicates visitor functionality" isn't "make all passes visitors", it's "make EraseRegions use a visitor".

Right, I don’t find use of Visitor for SimplifyCfg a good idea either; however I do not think MirPass is necessary yet – perhaps later, when we properly figure out our MIR passes story (see also ¹).

I’ve replaced Visitor implementation on SimplifyCfg with a inherent method.


I worry about future development

¹: The only pattern MirPass accommodates currently is running a pass on the whole MIR one at the time. I’ve been thinking/reading about dataflow analysis passes lately, and there’s a pattern of interleaving transformations from various passes inside a single dataflow analysis pass. IMO having a MirPass currently might inhibit implementation of such (good) dataflow analysis passes later.

@nagisa nagisa force-pushed the mir-transforms branch 3 times, most recently from 15ede21 to cf3318f Compare February 1, 2016 13:52
@nagisa nagisa changed the title Change MIR transformations to use MutVisitor Change EraseRegions pass to use MutVisitor Feb 1, 2016
@michaelwoerister
Copy link
Member

To give some context: I wrote EraseRegions at a time when MutVisitor did not exist yet. I'd certainly be in favor of using a visitor there.

@nikomatsakis
Copy link
Contributor

+1 to using a visitor, but I think I share @Aatch's concerns that we don't want to force all passes to use visitors.

@nikomatsakis
Copy link
Contributor

That said, I expect the MirPass stuff to evolve quite a bit, so I'm not too worried about it. But I guess people will try to shoehorn things into visitors if we hard-code that a visitor is the only sort of pass.

@nagisa
Copy link
Member Author

nagisa commented Feb 4, 2016

Greatly reduced scope to only contain move from custom visitor to MutVisitor for EraseRegions. Everything else will eventually come in different PRs.

@bors
Copy link
Contributor

bors commented Feb 6, 2016

☔ The latest upstream changes (presumably #31307) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2016

📌 Commit 0b3ef97 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 8, 2016

⌛ Testing commit 0b3ef97 with merge efdde24...

bors added a commit that referenced this pull request Feb 8, 2016
Having a `MirPass` provides literally no benefits over `MutVisitor`. Moreover using `MirPass` for
`EraseRegions` basically makes the programmer to fix breakage from changing repr twice – in the
visitor and eraseregions. Since `MutVisitor` implements all the “walking” inside the trait, that can
be reused for `EraseRegions` too, basically resulting in less code duplication.
@bors bors merged commit 0b3ef97 into rust-lang:master Feb 8, 2016
bors added a commit that referenced this pull request Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants