-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Eliminate use of #[cfg_attr(not(doc), repr(...))]
#116743
Conversation
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead. |
27411f9
to
56f472d
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.
This can't land as is, I'm afraid. My PR only affects the transparent representation. I've totally overlooked non-transparent reprs but in my defense nobody has mentioned this either over at the issue and the PR.
It does make sense though to extend this behavior to other reprs 🤦. I will open a PR as soon as possible.
CC @RalfJung
@@ -204,7 +204,7 @@ mod c_long_definition { | |||
// be UB. | |||
#[doc = include_str!("c_void.md")] | |||
#[lang = "c_void"] | |||
#[cfg_attr(not(doc), repr(u8))] // work around https://github.com/rust-lang/rust/issues/90435 | |||
#[repr(u8)] |
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.
Unfortunately, this will still show up in the docs, my PR only affects repr(transparent)
. Further, it doesn't take into account #[doc(hidden)]
(in this case on variants). Arguably, rustdoc should hide #[repr(u8)]
and respect doc(hidden)
. I can send a PR right away.
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.
Shouldn't we keep non-transparent reprs for enums though? It's useful, especially repr(c)
ones for FFI.
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.
For sure, but in this case both variants are doc(hidden)
. I plan on hiding those repr
s if all variants are hidden.
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.
Good idea!
@@ -297,7 +297,7 @@ impl<'f> fmt::Debug for VaListImpl<'f> { | |||
not(target_os = "uefi"), | |||
not(windows), | |||
))] | |||
#[cfg_attr(not(doc), repr(C))] // work around https://github.com/rust-lang/rust/issues/66401 | |||
#[repr(C)] |
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.
Similarly, repr(C)
is still going to show up in the docs :| Let me update rustdoc to handle that as well.
…rk-Simulacrum Request that rust-analyzer changes are sent upstream first if possible This automates `@lnicola's` comment rust-lang#118253 (comment). Rustbot will write a comment similar to rust-lang#116743 (comment).
Rollup merge of rust-lang#118255 - dtolnay:mentionsrustanalyzer, r=Mark-Simulacrum Request that rust-analyzer changes are sent upstream first if possible This automates `@lnicola's` comment rust-lang#118253 (comment). Rustbot will write a comment similar to rust-lang#116743 (comment).
I'll rebase and reopen if #116882 makes progress. |
This PR reverts #107680 and some pre-existing occurrences of the same pattern.
The issue that prompted the workaround, #90435, has since been fixed by #115439.
Tested by running
x.py doc --stage 1 library/std
and confirming that std/path/struct.PathBuf.html contains no visible#[repr(transparent)]
. Note thatx.py doc library/std
runs using stage0 rustdoc (maybe depending on config.toml?) and will continue to render a#[repr(transparent)]
, but that's okay and shouldn't make it into official documentation of the 1.75.0 release.Blocked on #116882