-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Exclude "test" from doc_auto_cfg #92856
Exclude "test" from doc_auto_cfg #92856
Conversation
Some changes occurred in cc @camelid |
f985f66
to
a03d615
Compare
src/librustdoc/clean/types.rs
Outdated
.filter_map(|attr| match Cfg::parse_without(attr.meta_item()?, &[sym::test]) { | ||
Ok(Some(c)) => Some(c), | ||
_ => None, | ||
}) | ||
.filter(|cfg| !hidden_cfg.contains(cfg)) |
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.
Couldn't we just add test
into the hidden_cfg
?
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.
Ah wait, that would need the other change to apply the hiding to sub-expressions rather than the expression as a whole.
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.
Yep, hence why I didn't. ;)
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.
I wonder if it would be good to make that change first, so we don't need the extra code here specifically to handle test
?
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.
You mean putting test
in hidden_cfg
and then simply pass hidden_cfg
to Cfg::parse_without
instead of &[sym::test]
?
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.
Yep, something like that.
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.
Ok!
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.
@Nemo157: That won't be possible as is because cfg_hide
is to hide explicitly a cfg
(any(not(feature = "miri-test-libstd"), test, doctest)
for example) whereas this PR is to remove any appearances of test
. The goals are completely different.
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.
Not really, test
is just a very simple cfg-expression that could be hidden as more complex expressions can. As mentioned on the linked issue we can have cfg_hide
hide arbitrary sub-expressions of the overall cfg
. So e.g. if test
is hidden any(not(test), foo)
would simplify to foo
, and if both test
and feature = "__internal"
are hidden then any(test, feature = "__internal")
would simplify to nothing at all.
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.
Hum ok... Let me give it another try.
r? @Nemo157 |
a03d615
to
741d75d
Compare
I rebased on top of master but didn't change anything else otherwise. |
@Nemo157 It now removes |
This comment has been minimized.
This comment has been minimized.
f2e1bca
to
9150911
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.
Couple of minor things that I think could be simplified, other than that looks great to me.
r=me with those simplified
9150911
to
b0df765
Compare
@bors: r=Nemo157 |
📌 Commit b0df765 has been approved by |
…askrgr Rollup of 17 pull requests Successful merges: - rust-lang#91032 (Introduce drop range tracking to generator interior analysis) - rust-lang#92856 (Exclude "test" from doc_auto_cfg) - rust-lang#92860 (Fix errors on blanket impls by ignoring the children of generated impls) - rust-lang#93038 (Fix star handling in block doc comments) - rust-lang#93061 (Only suggest adding `!` to expressions that can be macro invocation) - rust-lang#93067 (rustdoc mobile: fix scroll offset when jumping to internal id) - rust-lang#93086 (Add tests to ensure that `let_chains` works with `if_let_guard`) - rust-lang#93087 (Fix src/test/run-make/raw-dylib-alt-calling-convention) - rust-lang#93091 (⬆ chalk to 0.76.0) - rust-lang#93094 (src/test/rustdoc-json: Check for `struct_field`s in `variant_tuple_struct.rs`) - rust-lang#93098 (Show a more informative panic message when `DefPathHash` does not exist) - rust-lang#93099 (rustdoc: auto create output directory when "--output-format json") - rust-lang#93102 (Pretty printer algorithm revamp step 3) - rust-lang#93104 (Support --bless for pp-exact pretty printer tests) - rust-lang#93114 (update comment for `ensure_monomorphic_enough`) - rust-lang#93128 (Add script to prevent point releases with same number as existing ones) - rust-lang#93136 (Backport the 1.58.1 release notes to master) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #91740.
cc @Nemo157 (you were the one suggesting this iirc)
r? @camelid