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.

Sign up for GitHub

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

Jump to bottom

hir: replace "items" terminology with "nodes" where appropriate. #70092

Merged
merged 2 commits into from
Mar 21, 2020
Merged

hir: replace "items" terminology with "nodes" where appropriate. #70092

merged 2 commits into from
Mar 21, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Mar 18, 2020 •
edited
Loading

The newly added HirOwnerItems confused me before I realized that "items" there actually referred to HIR nodes, not hir:Item or "item-like" (which we should IMO replace with "owner").

I suspect the naming had something to do with ItemLocalId's use of "item".
That is, ItemLocalId could be interpreted to mean one of two things:

  • IntraItemNodeId i.e. IntraOwnerNodeId
    • this is IMO correct, and I'd even like to rename it, but I didn't want to throw that into this PR
  • IntraOwnerItemId
    • this is what HirOwnerItems would seem to imply

r? @Zoxc cc @michaelwoerister @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2020
Zoxc
Zoxc reviewed Mar 18, 2020
View reviewed changes
src/librustc/hir/map/mod.rs Outdated Show resolved Hide resolved
Zoxc
Zoxc reviewed Mar 18, 2020
View reviewed changes
src/librustc/query/mod.rs Outdated
@@ -70,19 +70,19 @@ rustc_queries! {
eval_always
}

// An HIR item with a `DefId` that can own other HIR items which do not themselves have
// An HIR node with a `DefId` that can own other HIR nodes which do not themselves have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments turned out to be inaccurate since some nodes have both a DefId and a HirId without being a HIR owner themselves. Maybe fix them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments turned out to be inaccurate since some nodes have both a DefId and a HirId without being a HIR owner themselves. Maybe fix them?

Right, currently only a subset of DefIds are HIR owners. I'll take a second look at these comments to try and make that clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried something, but whew that phrasing is hard to use.

I would be happier with hir::Owner (or I guess hir::OwnerKind) being a subset of Node and explicitly listing all of the supported node types.

Then it sort of self-documents what kinds of nodes are owners.
And I could also use it to add a visit_owner method to HIR visitors that would work with exhaustive matching.

But I didn't want to put any changes like that into this tiny PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably better to assume that the reader know what a HIR owner is here. I can barely make sense of the comments and I wrote the code =P

@bors

This comment has been minimized.

Sign in to view
@eddyb
Copy link
Member Author

eddyb commented Mar 19, 2020

cc @petrochenkov (since you've reviewed my recent changes around HIR)

@Zoxc
Copy link
Contributor

Zoxc commented Mar 19, 2020

The PR looks good to me excluding the comments on the queries.

Zoxc
Zoxc reviewed Mar 19, 2020
View reviewed changes
src/librustc/query/mod.rs Outdated Show resolved Hide resolved
Zoxc
Zoxc reviewed Mar 19, 2020
View reviewed changes
src/librustc/query/mod.rs Outdated Show resolved Hide resolved
@Zoxc
Copy link
Contributor

Zoxc commented Mar 19, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2020

📌 Commit be9679d has been approved by Zoxc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2020
@Centril Centril mentioned this pull request Mar 21, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2020
@bors
Auto merge of rust-lang#70211 - Centril:rollup-fusltgc, r=Centril
98803c1 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#67888 (Prefetch some queries used by the metadata encoder)
 - rust-lang#69934 (Update the mir inline costs)
 - rust-lang#69965 (Refactorings to get rid of rustc_codegen_utils)
 - rust-lang#70054 (Build dist-android with --enable-profiler)
 - rust-lang#70089 (rustc_infer: remove InferCtxt::closure_sig as the FnSig is always shallowly known.)
 - rust-lang#70092 (hir: replace "items" terminology with "nodes" where appropriate.)
 - rust-lang#70138 (do not 'return' in 'throw_' macros)
 - rust-lang#70151 (Update stdarch submodule)

Failed merges:

 - rust-lang#70074 (Expand: nix all fatal errors)

r? @ghost
@bors bors merged commit 569272a into rust-lang:master Mar 21, 2020
@eddyb eddyb deleted the hir-items-are-just-nodes branch March 21, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zoxc Zoxc Zoxc left review comments

Assignees

@Zoxc Zoxc

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eddyb @bors @Zoxc @rust-highfive