-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
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 " /cc @rust-lang/compiler am I over-engineering this? |
Right, I don’t find use of I’ve replaced Visitor implementation on SimplifyCfg with a inherent method.
¹: The only pattern |
15ede21
to
cf3318f
Compare
To give some context: I wrote |
+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. |
That said, I expect the |
cf3318f
to
446370b
Compare
Greatly reduced scope to only contain move from custom visitor to MutVisitor for EraseRegions. Everything else will eventually come in different PRs. |
446370b
to
cedf9b5
Compare
☔ The latest upstream changes (presumably #31307) made this pull request unmergeable. Please resolve the merge conflicts. |
cedf9b5
to
0b3ef97
Compare
@bors r+ |
📌 Commit 0b3ef97 has been approved by |
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.
Having a
MirPass
provides literally no benefits overMutVisitor
. Moreover usingMirPass
forEraseRegions
basically makes the programmer to fix breakage from changing repr twice – in thevisitor and eraseregions. Since
MutVisitor
implements all the “walking” inside the trait, that canbe reused for
EraseRegions
too, basically resulting in less code duplication.