-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Tweak DefPathData
#139662
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
Tweak DefPathData
#139662
Conversation
No need to convert the `DefKind` to `DefPathData`, they're very similar types.
PR rust-lang#137977 changed `DefPathData::TypeNs` to contain `Option<Symbol>` to account for RPITIT assoc types being anonymous. This commit changes it back to `Symbol` and gives anonymous assoc types their own variant. It makes things a bit nicer overall.
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in compiler/rustc_sanitizers cc @rcvalle |
DefKind::AssocTy => { | ||
if let Some(name) = name { | ||
DefPathData::TypeNs(name) | ||
} else { | ||
DefPathData::AnonAssocTy | ||
} | ||
} |
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 know it's a matter of taste, but I find the following more compact and as readable,feel free to ignore:
DefKind::AssocTy => { | |
if let Some(name) = name { | |
DefPathData::TypeNs(name) | |
} else { | |
DefPathData::AnonAssocTy | |
} | |
} | |
DefKind::AssocTy => name.map_or(DefPathData::AnonAssocTy, DefPathData::TypeNs), |
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 prefer the if
statement
This comment has been minimized.
This comment has been minimized.
Give them their own symbol `anon_assoc`, as is done for all the other anonymous `DefPathData` variants.
9e29ae3
to
cdf5b8d
Compare
@@ -451,6 +446,7 @@ impl DefPathData { | |||
Ctor => DefPathDataName::Anon { namespace: sym::constructor }, | |||
AnonConst => DefPathDataName::Anon { namespace: sym::constant }, | |||
OpaqueTy => DefPathDataName::Anon { namespace: sym::opaque }, | |||
AnonAssocTy => DefPathDataName::Anon { namespace: sym::anon_assoc }, |
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'm not 100% certain about the name anon_assoc
, happy to hear alternative suggestions.
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'm not a huge fan of the name, but not enough to try to think of a new name. Ideally we wouldn't render this at all in diagnostics, but special casing all diagnostics that render paths would be difficult b/c we'd need to work out an alternative to rendering the path of the anonymous GAT.
r? compiler-errors @bors r+ rollup |
Rollup of 7 pull requests Successful merges: - rust-lang#137835 (Use `BinOp::Cmp` for `iNN::signum`) - rust-lang#139584 (Avoid a reverse map that is only used in diagnostics paths) - rust-lang#139638 (Cleanup the `InstSimplify` MIR transformation) - rust-lang#139653 (Handle a negated literal in `eat_token_lit`.) - rust-lang#139662 (Tweak `DefPathData`) - rust-lang#139664 (Reuse address-space computation from global alloc) - rust-lang#139687 (Add spastorino to users_on_vacation) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#139662 - nnethercote:tweak-DefPathData, r=compiler-errors Tweak `DefPathData` Some improvements in and around `DefPathData`, following on from rust-lang#137977. r? `@spastorino`
…ompiler-errors Tweak `DefPathData` Some improvements in and around `DefPathData`, following on from rust-lang#137977. r? `@spastorino`
Some improvements in and around
DefPathData
, following on from #137977.r? @spastorino