Change visit api #61554
Merged
Change visit api #61554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
I've processed the rest of the So this ends being a little change that I think it's a bit better but may not worth applying, unsure to be honest :). |
|
In particular, having |
I think just leaving a comment on the |
I like it, and we're going to "need it" (in some form) anyway when moving to slice projections. |
oli-obk
approved these changes
Jun 6, 2019
spastorino
force-pushed
the
change_visit_api
branch
from
e786f63
to
67197e2
Compare
|
@bors r+ |
|
📌 Commit 67197e2 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 6, 2019
Change visit api r? @oli-obk In the [first commit](rust-lang@37386d3) of this PR, I'm changing `visit_place` to be the function that traverses the `Place` and have only that responsibility. Then there are two other functions `visit_place_base` and `visit_projection` which are the ones in charge of visiting the base and the projection. Visitor implementors can implement any of those. In the [second commit](rust-lang@e786f63) we can already see some things that confuses me, which I think this division will make more clear. The old code, first checked if the place was a base, did something with it and then called `super_place` [here](rust-lang@e786f63#diff-d583e4efe1a72516e274158e53223633L678). `super_place` checks again if it's a base [here](https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L679-L684) and in case is a local, visits the local and stuff like that. That's not very obvious on the code, and if I'm not wrong it's not needed. In this PR or we have [this](rust-lang@e786f63#diff-d583e4efe1a72516e274158e53223633R673) as I did or we can just do `- => self.super_place_base(...)` and that will be obvious that I'm letting the default implementation process the base.
|
⌛ Testing commit 67197e2 with merge 8d380c4fe5c3cf00b5231cdd14d49bf50d9f5996... |
|
💔 Test failed - checks-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
|
The rand crate broke. |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jun 6, 2019
Change visit api r? @oli-obk In the [first commit](rust-lang@37386d3) of this PR, I'm changing `visit_place` to be the function that traverses the `Place` and have only that responsibility. Then there are two other functions `visit_place_base` and `visit_projection` which are the ones in charge of visiting the base and the projection. Visitor implementors can implement any of those. In the [second commit](rust-lang@e786f63) we can already see some things that confuses me, which I think this division will make more clear. The old code, first checked if the place was a base, did something with it and then called `super_place` [here](rust-lang@e786f63#diff-d583e4efe1a72516e274158e53223633L678). `super_place` checks again if it's a base [here](https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L679-L684) and in case is a local, visits the local and stuff like that. That's not very obvious on the code, and if I'm not wrong it's not needed. In this PR or we have [this](rust-lang@e786f63#diff-d583e4efe1a72516e274158e53223633R673) as I did or we can just do `- => self.super_place_base(...)` and that will be obvious that I'm letting the default implementation process the base.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @oli-obk
In the first commit of this PR, I'm changing
visit_placeto be the function that traverses thePlaceand have only that responsibility. Then there are two other functionsvisit_place_baseandvisit_projectionwhich are the ones in charge of visiting the base and the projection. Visitor implementors can implement any of those.In the second commit we can already see some things that confuses me, which I think this division will make more clear. The old code, first checked if the place was a base, did something with it and then called
super_placehere.super_placechecks again if it's a base here and in case is a local, visits the local and stuff like that. That's not very obvious on the code, and if I'm not wrong it's not needed. In this PR or we have this as I did or we can just do- => self.super_place_base(...)and that will be obvious that I'm letting the default implementation process the base.