Lowering cleanups [1/N] #51806
Merged
Lowering cleanups [1/N] #51806
Conversation
This file contains hidden or 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
Contributor
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
r? @cramertj I touched a lot of the async lowering code |
This comment has been minimized.
This comment has been minimized.
bjorn3
requested changes
Jun 26, 2018
src/librustc/hir/map/collector.rs
Outdated
Collaborator
|
☔ The latest upstream changes (presumably #51805) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
force-pushed
the
lowering_cleanups1
branch
from
1ebe798 to
f68b1e7
Compare
Contributor
|
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 |
cramertj
reviewed
Jun 26, 2018
src/librustc/hir/lowering.rs
Outdated
Member
There was a problem hiding this comment.
Nit: why switch this to match?
cramertj
reviewed
Jun 26, 2018
src/librustc/hir/lowering.rs
Outdated
Member
There was a problem hiding this comment.
Can you leave a comment about what the NodeId is used for? At first glance I expected it to be the closure_node_id that is stored in IsAsync, and was surprised that it was actually for the impl Trait.
cramertj
reviewed
Jun 26, 2018
src/librustc/hir/lowering.rs
Outdated
Member
There was a problem hiding this comment.
I see this copy-pasted a few places-- can you break this out into a return_impl_trait_id method that returns an Option<NodeId>?
cramertj
reviewed
Jun 26, 2018
src/librustc/hir/lowering.rs
Outdated
Member
There was a problem hiding this comment.
Nit: this async impl trait id addition is copy-pasted below-- I feel like it belongs as part of lower_impl_trait_ids for the function in question. It's unfortunate that async means that the FnDecl on its own isn't enough to see all of the impl Trait instances-- perhaps have lower_impl_trait_ids also accept the FnHeader, or maybe just the IsAsync?
cramertj
reviewed
Jun 26, 2018
Member
There was a problem hiding this comment.
Nice-- I'm glad to see these moved to def_collector.
cramertj
reviewed
Jun 26, 2018
src/librustc/hir/intravisit.rs
Outdated
Member
There was a problem hiding this comment.
Can you leave a comment here explaining that we don't visit the impl Trait ID here because it's collected by lower_item_id for the function itself? (here)
Member
|
This is great! r=me with nits addressed. |
Collaborator
|
☔ The latest upstream changes (presumably #51773) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
force-pushed
the
lowering_cleanups1
branch
from
1fc4518 to
f21a98d
Compare
Contributor
Author
|
@bors r=cramertj |
Collaborator
|
📌 Commit f21a98d has been approved by |
bors
added
S-waiting-on-bors
Contributor
Author
|
@bors r=cramertj |
Collaborator
|
📌 Commit 99575b5 has been approved by |
Collaborator
|
⌛ Testing commit 99575b5 with merge 8edb2c121e01c78b5a8d52fb411b108cdf964e24... |
Collaborator
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
Contributor
|
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 |
1 similar comment
Contributor
|
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 |
Member
bors
added
S-waiting-on-bors
eddyb
reviewed
Jun 28, 2018
| // as part of the surrounding module. The `NodeId` just exists so we don't have to look | ||
| // it up everywhere else in the compiler | ||
| visitor.visit_def_mention(Def::Existential(def_id)); | ||
| visitor.visit_nested_item(item_id); |
Member
There was a problem hiding this comment.
Hang on, if the NodeId is pointing to the same thing as the DefId, why do we keep both?!
We only really need the DefId for most of the compilation, AFAIK, and the occasional need to get the NodeId from the DefId shouldn't be enough to keep the NodeId in the HIR.
Contributor
Author
There was a problem hiding this comment.
I have further refactorings planned, this entire variant is getting removed ASAP, so I didn't feel very motivated to make it idiomatic
Member
There was a problem hiding this comment.
Oh, huh, are you going with a fake path?
Contributor
Author
There was a problem hiding this comment.
Yea, not very fake though. The items are already real and in the parent of the function.
Member
There was a problem hiding this comment.
Yeah, I mean just not part of the original AST. Seems good!
Collaborator
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
bors
added a commit
that referenced
this pull request
Jul 1, 2018
Lowering cleanups [3/N] Needs #51806 to be merged first
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
No description provided.