Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 14, 2023

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 that x.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

@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 14, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2023

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

@dtolnay
Copy link
Member Author

dtolnay commented Oct 14, 2023

library/portable-simd/crates/core_simd/src/masks.rs change extracted to rust-lang/portable-simd#370.

Copy link
Member

@fmease fmease left a 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)]
Copy link
Member

@fmease fmease Oct 14, 2023

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.

Copy link
Member

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.

Copy link
Member

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 reprs if all variants are hidden.

Copy link
Member

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)]
Copy link
Member

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.

@dtolnay dtolnay added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2023
fmease added a commit to fmease/rust that referenced this pull request Nov 25, 2023
…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).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2023
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).
@dtolnay
Copy link
Member Author

dtolnay commented Jan 21, 2024

I'll rebase and reopen if #116882 makes progress.

@dtolnay dtolnay closed this Jan 21, 2024
@dtolnay dtolnay deleted the notdoctransparent branch January 21, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants