Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
| match kind { | ||
| AttributeKind::NoMangle(..) | ||
| | AttributeKind::ExportName { .. } | ||
| | AttributeKind::RustcStdInternalSymbol | ||
| | AttributeKind::RustcEiiForeignItem | ||
| | AttributeKind::EiiImpl(..) | ||
| | AttributeKind::RustcOffloadKernel => true, | ||
| AttributeKind::Linkage(linkage, _) => *linkage != rustc_attr_ir::Linkage::Internal, | ||
| AttributeKind::Lang(item) => item.link_name().is_some(), | ||
| _ => false, | ||
| } |
There was a problem hiding this comment.
This should be implemented more rigorously, can you refactor this match into a method on AttributeKind and match exhaustively there, similar to encode_cross_crate?
(and then update its and contains_extern_indicator's doc comments to link to each other, because these must be kept in sync)
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
Some changes occurred in compiler/rustc_attr_ir |
| LinkName { .. } => false, | ||
| LinkOrdinal { .. } => false, | ||
| LinkSection { .. } => false, | ||
| Linkage(linkage, _) => *linkage != crate::Linkage::Internal, |
There was a problem hiding this comment.
Can you also exhaustively match on Linkage here? The idea is that this should refuse to compile if anyone ever adds a new Linkage variant without updating this.
|
Otherwise it looks good, will wait for @JonathanBrouwer for a final review |
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
|
||
| /// Whether this attribute is an extern indicator. | ||
| /// | ||
| /// Must stay in sync with [`CodegenFnAttrs::contains_extern_indicator`]. |
There was a problem hiding this comment.
Hmmm, I'm not a big fan that we are duplicating this logic here.
The effort done to ensure these stay in sync is good, but it would be better not do to this at all.
Can we re-implement instantiation_mode using this method instead, and then remove CodegenFnAttrs::contains_extern_indicator?
There was a problem hiding this comment.
If we can do that, we can re-consider if we need to match exhaustively here. It might be nice to still do, but it's a lot less valuable then
rustbot
added
S-waiting-on-author
5 participants
Updates #153101
r? @JonathanBrouwer