Implement argument-dependent target checking for the #[repr] parser#157215
Implement argument-dependent target checking for the #[repr] parser#157215JonathanBrouwer wants to merge 5 commits into
#[repr] parser#157215Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
JonathanBrouwer
force-pushed
the
repr-target-checking3
branch
from
7758293 to
d58ea4a
Compare
|
I like this approach of using ManuallyChecked and asserting whether it was! Good solution :3 |
There was a problem hiding this comment.
Ugh, #[repr] gives me a headache.
Anyway, the implementation looks good generally.
What is the plan going forward? I imagine a series of prs like:
- macrocall checking for the other attrs in #156499
- some kind of "this attribute does nothing even if this macro call expands into a valid target" note on the warning
- a PR to raise the lint level to deny by default for repr attrs on macro calls and do something about the
repr(simd)thing
| @@ -1,8 +1,10 @@ | |||
| #### Note: this error code is no longer emitted by the compiler. | |||
|
|
|||
There was a problem hiding this comment.
Same with the other PR, please direct to the new error code.
There was a problem hiding this comment.
There is no new error code, the new diagnostic doesn't have one :p.
Added a comment saying this
| let (reprs, _first_attr_span) = | ||
| find_attr!(attrs, Repr { reprs, first_span } => (reprs.as_slice(), Some(*first_span))) | ||
| .unwrap_or((&[], None)); | ||
|
|
There was a problem hiding this comment.
| let (reprs, _first_attr_span) = | |
| find_attr!(attrs, Repr { reprs, first_span } => (reprs.as_slice(), Some(*first_span))) | |
| .unwrap_or((&[], None)); | |
| let reprs = find_attr!(attrs, Repr { reprs, .. } => reprs.as_slice()).unwrap_or(&[]); |
Also do we still need to keep first_span in the AttributeKind::Repr?
There was a problem hiding this comment.
Yes we do sadly :c
It is still used for a few diagnostics later, I might try moving those in a followup PR
This comment has been minimized.
This comment has been minimized.
|
(still need to actually push my changes that fixed your review comments, blocked on #157291) |
The first one was already my plan indeed, the other two also sound like great ideas :) |
…cking, r=mejrs Clean up attribute target checking diagnostics Split off from me trying to process your review comments in rust-lang#157215 Thanks to @GuillaumeGomez for making this possible <3 r? @mejrs
mejrs
added
S-waiting-on-author
Rollup merge of #157291 - JonathanBrouwer:cleanup-target-checking, r=mejrs Clean up attribute target checking diagnostics Split off from me trying to process your review comments in #157215 Thanks to @GuillaumeGomez for making this possible <3 r? @mejrs
JonathanBrouwer
force-pushed
the
repr-target-checking3
branch
from
d58ea4a to
2a8fee0
Compare
|
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. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
JonathanBrouwer
force-pushed
the
repr-target-checking3
branch
from
2a8fee0 to
79e666b
Compare
|
CI was already green, and the last fix should not affect that |
rust-bors
Bot
added
S-waiting-on-bors
…ng3, r=mejrs Implement argument-dependent target checking for the `#[repr]` parser Fixes some of rust-lang#156499 Fixes some of rust-lang#153101 Alternative approach to rust-lang#156569, which was suggested in the comments here: rust-lang#156569 (review) How do you feel about this approach? r? @mejrs
…uwer Rollup of 9 pull requests Successful merges: - #157035 (`LivenessValues`: use dedicated enum rather than mutually exclusive `Option`s) - #157182 (Restore simpler Encode/Decode impls for u32 and (on 64bit systems) usize) - #157310 (rustdoc: Render `impl` restriction) - #157070 (Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros) - #157137 (rustc_target: Use +spe for powerpcspe targets) - #157215 (Implement argument-dependent target checking for the `#[repr]` parser) - #157235 (additional changes for issue-144595) - #157321 (Remove unnecessary arm in `handle_res`) - #157324 (Add test for undefined EII static error)
4 participants
View all comments
Fixes some of #156499
Fixes some of #153101
Alternative approach to #156569, which was suggested in the comments here: #156569 (review)
How do you feel about this approach?
r? @mejrs