-
Notifications
You must be signed in to change notification settings - Fork 13.3k
HirIdification: add key HirId methods #58090
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
Conversation
10fb15c
to
3c0628f
Compare
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 |
The error was fixed. |
55227f8
to
54d937b
Compare
54d937b
to
e8aeb83
Compare
@Zoxc thanks for the thorough review! I guess I was a bit overzealous with moving away from Comments addressed. |
@Zoxc done; if everything is ok now, you can delegate the PR to me so I can later squash the correctional commit and approve it on your behalf afterwards. |
@bors delegate+ |
✌️ @ljedrz can now approve this pull request |
@bors r+ |
📌 Commit 272f4df has been approved by |
HirIdification: add key HirId methods This is another PR in a series dedicated to `HirId`-ification, i.e. deprecating `ast::NodeId`s after the AST > HIR lowering process. The bigger proof of concept can be seen in #57578. **Phase 2**: add key `HirId` methods mirroring the `NodeId` ones. These should be counterparts of the most widely used `Hir` methods using `NodeId`s. Note that this expands `hir::map::Definitions` with an additional `hir_to_def_index` map (with the intention of later removing `node_to_def_index`). As a bonus there is also a small cleanup commit removing unnecessary calls to `node_to_hir_id` where `HirId` is already available. r? @Zoxc Cc @varkor
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@4314dba. Direct link to PR: <rust-lang/rust#58090> 💔 rls on linux: test-pass → test-fail (cc @nrc @Xanewok, @rust-lang/infra).
@Zoxc sorry for not having squashed in time, I had to get some sleep. Hopefully the git historians won't mind ^^. I'll start working on switching the functions now. |
@@ -76,6 +77,11 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
self.item_path_str(self.hir().local_def_id(id)) | |||
} | |||
|
|||
// FIXME(@ljedrz): replace the NodeId variant | |||
pub fn hir_path_str(self, id: hir::HirId) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use this - #57967 (or the refactoring that it's based on, rather, will split it into a separate PR) removes node_path_str
.
If you really need to replace node_path_str
uses, you can call item_path_str
directly (a lot of the time you might even have a DefId
that was being converted to a NodeId
!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb thanks for the heads up, I'll keep that in mind; I'll also remove this function with a drive-by commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #58140 in the meanwhile, and it removes hir_path_str
, so it's compatible with any PR also removing hir_path_str
.
This is another PR in a series dedicated to
HirId
-ification, i.e. deprecatingast::NodeId
s after the AST > HIR lowering process. The bigger proof of concept can be seen in #57578.Phase 2: add key
HirId
methods mirroring theNodeId
ones.These should be counterparts of the most widely used
Hir
methods usingNodeId
s. Note that this expandshir::map::Definitions
with an additionalhir_to_def_index
map (with the intention of later removingnode_to_def_index
).As a bonus there is also a small cleanup commit removing unnecessary calls to
node_to_hir_id
whereHirId
is already available.r? @Zoxc
Cc @varkor