nr2.0: Separate out canonical path handling #3776
Open
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
powerboat9
force-pushed
the
fix-can
branch
4 times, most recently
from
326ac5d to
1156370
Compare
|
|
powerboat9
requested review from
P-E-P,
CohenArthur and
philberty
and removed request for
P-E-P
This should improve our canonical path handling, without requiring further tweaks to ForeverStack. This may also help if, in the future, we have to move canonical path calculation to later compilation phases for proper handling of generics. gcc/rust/ChangeLog: * backend/rust-compile-base.cc (HIRCompileBase::compile_function): Since canonical paths returned from nr2.0 now include the crate name, avoid prepending the crate name again. * backend/rust-compile-implitem.cc (CompileTraitItem::visit): Use NameResolutionContext::to_canonical_path instead of ForeverStack::to_canonical_path. * backend/rust-compile-item.cc (CompileItem::visit): Likewise. * typecheck/rust-hir-type-check-enumitem.cc (TypeCheckEnumItem::visit): Likewise. * typecheck/rust-hir-type-check-implitem.cc (TypeCheckImplItem::visit): Likewise. * typecheck/rust-hir-type-check-item.cc (TypeCheckItem::visit): Likewise. * typecheck/rust-hir-type-check.cc (TraitItemReference::get_type_from_fn): Likewise. * resolve/rust-default-resolver.cc (DefaultResolver::visit): Add Crate and EnumItem instance visitors, handle canonical path context scoping. * resolve/rust-default-resolver.h (DefaultResolver::visit): Add Crate and EnumItem instance visitors. * resolve/rust-early-name-resolver-2.0.cc (Early::go): Visit instances of Crate using the virtual member function visit. * resolve/rust-forever-stack.h (ForeverStack::to_canonical_path): Remove function declaration. * resolve/rust-forever-stack.hxx (ForeverStack::to_canonical_path): Remove function definition. * resolve/rust-late-name-resolver-2.0.cc (Late::go): Visit instances of Crate using the virtual member function visit. * resolve/rust-name-resolution-context.cc (CanonicalPathRecordRoot::as_path): New function definition. (CanonicalPathRecordNormal::as_path): Likewise. (CanonicalPathRecordLookup::as_path): Likewise. (CanonicalPathRecordImpl::as_path): Likewise. (CanonicalPathRecordTraitImpl::as_path): Likewise. (NameResolutionContext::NameResolutionContext): Initialize member variable canonical_ctx. * resolve/rust-name-resolution-context.h: Include "rust-item.h". (class NameResolutionContext): Forward declare class. (class CanonicalPathRecord): New class. (class CanonicalPathRecordWithParent): Likewise. (class CanonicalPathRecordRoot): Likewise. (class CanonicalPathRecordNormal): Likewise. (class CanonicalPathRecordLookup): Likewise. (class CanonicalPathRecordImpl): Likewise. (class CanonicalPathRecordTraitImpl): Likewise. (class CanonicalPathCtx): Likewise. (NameResolutionContext::canonical_ctx): New member variable. (NameResolutionContext::to_canonical_path): New member function. * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::go): Visit instances of Crate with the virtual member function visit. (TopLevel::visit): Handle canonical path context scoping for external crates, use DefaultResolver::visit when visiting instances of StructStruct. * util/rust-canonical-path.h (CanonicalPath::new_seg): Take path parameter by-value, as a duplicate instance will be constructed regardless. gcc/testsuite/ChangeLog: * rust/compile/nr2/exclude: Remove canonical_paths1.rs.
philberty
reviewed
May 13, 2025
| // this should bring us roughly to parity with nr1.0 | ||
| // since nr1.0 doesn't seem to handle canonical paths for generics | ||
| // quite right anyways | ||
| return Resolver::CanonicalPath::new_seg (UNKNOWN_NODEID, "XXX"); |
There was a problem hiding this comment.
yeah generics are a right pain to handle sometims the paths always default to their non monomorphized version sometimes not but yeah. In order to do it all "correctly" there is going to need to be some mappings between the segment and the conical path segments.. We get away with it for now because it doesnt matter, plus rust will never get a stable ABI and the hash at the end of the managled symbol.
My current feel is that if we can do what we can with canonical paths for now purely in name resolution we get like 80% the way there and its ok.
philberty
reviewed
May 13, 2025
| @@ -57,10 +57,11 @@ class CanonicalPath | |||
| return *this; | |||
| } | |||
|
|
|||
| static CanonicalPath new_seg (NodeId id, const std::string &path) | |||
| static CanonicalPath new_seg (NodeId id, std::string path) | |||
There was a problem hiding this comment.
Not sure i like getting rid of the const reference here. I personally like to avoid move on std::string but if this is the style in nr2 then go for it.
There was a problem hiding this comment.
I was having some issue with binding to a constant lvalue reference -- in hindsight, I'm not sure why
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.
Depends on a few other PRs