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

Simplify doc-cfg rendering based on the current context #77672

Merged
merged 3 commits into from
Oct 16, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Oct 7, 2020

For sub-items on a page don't show cfg that has already been rendered on
a parent item. At its simplest this means not showing anything that is
shown in the portability message at the top of the page, but also for
things like fields of an enum variant if that variant itself is
cfg-gated then don't repeat those cfg on each field of the variant.

This does not touch trait implementation rendering, as that is more
complex and there are existing issues around how it deals with doc-cfg
that need to be fixed first.

Screenshots, left is current, right is new:

image

image

image

image

image

cc #43781

For sub-items on a page don't show cfg that has already been rendered on
a parent item. At its simplest this means not showing anything that is
shown in the portability message at the top of the page, but also for
things like fields of an enum variant if that variant itself is
cfg-gated then don't repeat those cfg on each field of the variant.

This does not touch trait implementation rendering, as that is more
complex and there are existing issues around how it deals with doc-cfg
that need to be fixed first.
@rust-highfive
Copy link
Collaborator

r? @ollie27

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2020
@jyn514 jyn514 added A-rustdoc-ui Area: rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 8, 2020
src/librustdoc/clean/cfg.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks really good, thanks for working on it :)

cx: &Context,
item: &clean::Item,
is_hidden: bool,
parent: Option<&clean::Item>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional this only works with one level of nesting? I'm thinking of three nested modules, each requiring their own feature.
No problem if so, just wanted to make you'd thought of it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cfg in that case is pre-merged, if you look at the second screenshot above the feature="futures-io" cfg is coming from the async_compression::futures::bufread module, and the feature="brotli" cfg is coming from the BrotliDecoder, and these are merged into a single all(feature="futures-io", feature="brotli") cfg on BrotliDecoder. So for simple cases like this, the existing code merging these features down will handle nesting well (and I have yet to see any real code that wants to use non-simple cases).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for simple cases like this, the existing code merging these features down will handle nesting well (and I have yet to see any real code that wants to use non-simple cases).

That seems fine - if someone needs the complicated cases we can always add them in a follow-up, but I doubt it will come up.

@jyn514 jyn514 assigned jyn514 and unassigned ollie27 Oct 15, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2020

📌 Commit 4409cb2 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 15, 2020
Simplify doc-cfg rendering based on the current context

For sub-items on a page don't show cfg that has already been rendered on
a parent item. At its simplest this means not showing anything that is
shown in the portability message at the top of the page, but also for
things like fields of an enum variant if that variant itself is
cfg-gated then don't repeat those cfg on each field of the variant.

This does not touch trait implementation rendering, as that is more
complex and there are existing issues around how it deals with doc-cfg
that need to be fixed first.

### Screenshots, left is current, right is new:

![image](https://user-images.githubusercontent.com/81079/95387261-c2e6a200-08f0-11eb-90d4-0a9734acd922.png)

![image](https://user-images.githubusercontent.com/81079/95387458-06411080-08f1-11eb-81f7-5dd7f37695dd.png)

![image](https://user-images.githubusercontent.com/81079/95387702-6637b700-08f1-11eb-82f4-46b6cd9b24f2.png)

![image](https://user-images.githubusercontent.com/81079/95387905-b9aa0500-08f1-11eb-8d95-8b618d31d419.png)

![image](https://user-images.githubusercontent.com/81079/95388300-5bc9ed00-08f2-11eb-9ac9-b92cbdb60b89.png)

cc rust-lang#43781
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#75023 (ensure arguments are included in count mismatch span)
 - rust-lang#75265 (Add `str::{Split,RSplit,SplitN,RSplitN,SplitTerminator,RSplitTerminator,SplitInclusive}::as_str` methods)
 - rust-lang#75675 (mangling: mangle impl params w/ v0 scheme)
 - rust-lang#76084 (Refactor io/buffered.rs into submodules)
 - rust-lang#76119 (Stabilize move_ref_pattern)
 - rust-lang#77493 (ICEs should always print the top of the query stack)
 - rust-lang#77619 (Use futex-based thread-parker for Wasm32.)
 - rust-lang#77646 (For backtrace, use StaticMutex instead of a raw sys Mutex.)
 - rust-lang#77648 (Static mutex is static)
 - rust-lang#77657 (Cleanup cloudabi mutexes and condvars)
 - rust-lang#77672 (Simplify doc-cfg rendering based on the current context)
 - rust-lang#77780 (rustc_parse: fix spans on cast and range exprs with attrs)
 - rust-lang#77935 (BTreeMap: make PartialCmp/PartialEq explicit and tested)
 - rust-lang#77980 (Fix intra doc link for needs_drop)

Failed merges:

r? `@ghost`
@bors bors merged commit 71b0ea6 into rust-lang:master Oct 16, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 16, 2020
@jyn514 jyn514 added the F-doc_cfg `#![feature(doc_cfg)]` label Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: rustdoc UI (generated HTML) F-doc_cfg `#![feature(doc_cfg)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants