-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Initial impl of repr_packed_without_abi
#13398
Conversation
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
☔ The latest upstream changes (presumably #13286) made this pull request unmergeable. Please resolve the merge conflicts. |
"item uses `packed` representation without ABI-qualification", | ||
|diag| { | ||
diag.warn("unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI") | ||
.help("qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`") |
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.
Sorry for the wait!
Before 1.74 you couldn't write repr(Rust)
, adding an MSRV check is an option but I think leaving it out is reasonable too since you could just disable the lint if you want repr(Rust, packed)
and are targeting older versions
I'll leave that choice up to you, the rest looks great
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.
Force-pushed a rebase. I've added a msrv filter, although I'm not sure given the docs if msrv_aliases!
is supposed to only name actual feature-names as used in rustc; as far as I can see, #[repr(Rust)]
didn't get a name, so I made something up. Bumped to 1.84. Had to rebase beyond 63d0ba9, and I'm unsure if PostExpansionEarlyAttributes
is where this lint should live according to 63d0ba9's intent.
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.
It should be fine to go in Attributes
, the late pass
MSRV name is 👍, just has to be understandable
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.
Moved it over
c25cda9
to
d6e7956
Compare
d6e7956
to
7a80f7b
Compare
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.
Thanks!
Fixes #13375
I've added the lint next to the other attribute-related ones. Not sure if this is the correct place, since while we are looking after the
packed
-attribute (there is nothing we can do about types defined elsewhere), we are more concerned about the type's representation set by the attribute (instead of "duplicate attributes" and such).The lint simply looks at the attributes themselves without concern for the item-kind, since items where
repr
is not allowed end up in a compile-error anyway.I'm somewhat concerned about the level of noise this lint would cause if/when it goes into stable, although it does not come up in
lintcheck
.