cfg_select: Support unbraced expressions#145233
Conversation
When operating on expressions, `cfg_select!` can now handle expressions without braces. (It still requires braces for other things, such as items.) Expand the test coverage and documentation accordingly.
|
We discussed this in the @rust-lang/libs-api meeting and everyone was happy with this change. |
|
We discussed this in the lang meeting. We were all happy to see this land in nightly. Thanks to @joshtriplett for putting this forward. |
|
I updated the PR description to point to the tracking issue #115585. It seems like the tracking issue was very outdated, so I tried to update that as well (but I probably missed a few still). You may wish to update the tracking issue example (#115585 (comment)) to demonstrate this syntax allowance. |
jieyouxu
left a comment
There was a problem hiding this comment.
Implementation wise, this looks good, thanks!
|
@bors r+ rollup |
…eyouxu cfg_select: Support unbraced expressions Tracking issue for `cfg_select`: rust-lang#115585 When operating on expressions, `cfg_select!` can now handle expressions without braces. (It still requires braces for other things, such as items.) Expand the test coverage and documentation accordingly. --- I'm not sure whether deciding to extend `cfg_select!` in this way is T-lang or T-libs-api. I've labeled for both, with the request that both teams don't block on each other. :)
Rollup of 9 pull requests Successful merges: - #118087 (Add Ref/RefMut try_map method) - #140794 (Add information about group a lint belongs to) - #144947 (Fix description of unsigned `checked_exact_div`) - #145005 (strip prefix of temporary file names when it exceeds filesystem name length limit) - #145233 (cfg_select: Support unbraced expressions) - #145243 (take attr style into account in diagnostics) - #145353 (bootstrap: Fix jemalloc 64K page support for aarch64 tools) - #145379 (bootstrap: Support passing `--timings` to cargo) - #145389 ([rustdoc] Revert "rustdoc search: prefer stable items in search results") Failed merges: - #144983 (Rehome 37 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`) - #145065 (resolve: Introduce `RibKind::Block`) r? `@ghost` `@rustbot` modify labels: rollup
…eyouxu cfg_select: Support unbraced expressions Tracking issue for `cfg_select`: rust-lang#115585 When operating on expressions, `cfg_select!` can now handle expressions without braces. (It still requires braces for other things, such as items.) Expand the test coverage and documentation accordingly. --- I'm not sure whether deciding to extend `cfg_select!` in this way is T-lang or T-libs-api. I've labeled for both, with the request that both teams don't block on each other. :)
Done. |
Rollup of 11 pull requests Successful merges: - #137872 (Include whitespace in "remove |" suggestion and make it hidden) - #144631 (Fix test intrinsic-raw_eq-const-bad for big-endian) - #145233 (cfg_select: Support unbraced expressions) - #145261 (Improve tracing in bootstrap) - #145324 (Rename and document `ONLY_HOSTS` in bootstrap) - #145353 (bootstrap: Fix jemalloc 64K page support for aarch64 tools) - #145379 (bootstrap: Support passing `--timings` to cargo) - #145397 (Rust documentation, use `rustc-dev-guide` :3) - #145398 (Use `default_field_values` in `Resolver`) - #145401 (cleanup: Remove useless `[T].iter().last()`) - #145403 (Adjust error message grammar to be less awkward) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145233 - joshtriplett:cfg-select-expr, r=jieyouxu cfg_select: Support unbraced expressions Tracking issue for `cfg_select`: #115585 When operating on expressions, `cfg_select!` can now handle expressions without braces. (It still requires braces for other things, such as items.) Expand the test coverage and documentation accordingly. --- I'm not sure whether deciding to extend `cfg_select!` in this way is T-lang or T-libs-api. I've labeled for both, with the request that both teams don't block on each other. :)
|
@rust-timer build b7907e9 Testing for #145407. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (b7907e9): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.4%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.2%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 470.516s -> 470.62s (0.02%) |
|
The perf results there don't seem particularly likely from this PR; looking at some of the deltas they're in places like LLVM codegen, which this doesn't even touch. Are those spurious/noise? |
Rollup of 11 pull requests Successful merges: - rust-lang/rust#137872 (Include whitespace in "remove |" suggestion and make it hidden) - rust-lang/rust#144631 (Fix test intrinsic-raw_eq-const-bad for big-endian) - rust-lang/rust#145233 (cfg_select: Support unbraced expressions) - rust-lang/rust#145261 (Improve tracing in bootstrap) - rust-lang/rust#145324 (Rename and document `ONLY_HOSTS` in bootstrap) - rust-lang/rust#145353 (bootstrap: Fix jemalloc 64K page support for aarch64 tools) - rust-lang/rust#145379 (bootstrap: Support passing `--timings` to cargo) - rust-lang/rust#145397 (Rust documentation, use `rustc-dev-guide` :3) - rust-lang/rust#145398 (Use `default_field_values` in `Resolver`) - rust-lang/rust#145401 (cleanup: Remove useless `[T].iter().last()`) - rust-lang/rust#145403 (Adjust error message grammar to be less awkward) r? `@ghost` `@rustbot` modify labels: rollup
…eyouxu cfg_select: Support unbraced expressions Tracking issue for `cfg_select`: rust-lang#115585 When operating on expressions, `cfg_select!` can now handle expressions without braces. (It still requires braces for other things, such as items.) Expand the test coverage and documentation accordingly. --- I'm not sure whether deciding to extend `cfg_select!` in this way is T-lang or T-libs-api. I've labeled for both, with the request that both teams don't block on each other. :)
|
A few days later looking at the results of the parent rollup (https://perf.rust-lang.org/compare.html?start=898aff704d6f0d00343f21d31b8b9bfac8e43007&end=3507a749b365aae4eefa96ab700a9315d3280ee7&stat=instructions:u) in context of the subsequent perf, I think most of the affected benchmarks became much noisier around the time of this PR (not quite at the same time though, best as I can tell). So I don't think we should look more into this particular PR at this time. |
| if !classify::expr_is_complete(&expr) && p.token != token::CloseBrace && p.token != token::Eof { | ||
| p.expect(exp!(Comma))?; | ||
| } else { | ||
| let _ = p.eat(exp!(Comma)); | ||
| } |
There was a problem hiding this comment.
@joshtriplett what is happening here? @ehuss asked over at rust-lang/reference#2103 (comment) why the p.token != token::CloseBrace is needed here (given that the { /* ... */ } case is already handled above).
I suspect this is an attempt at good error messages? That isn't tested at all though.
…=JonathanBrouwer stabilize `cfg_select!` *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/149783)* tracking issue: rust-lang#115585 closes rust-lang#115585 reference PR: - rust-lang/reference#2103 # Request for Stabilization ## Summary The `cfg_select!` macro picks the expansion corresponding to the first `cfg` condition that evaluates to `true`. It simplifies complex conditional expressions. ```rust cfg_select! { unix => { fn foo() { /* unix specific functionality */ } } target_pointer_width = "32" => { fn foo() { /* non-unix, 32-bit functionality */ } } _ => { fn foo() { /* fallback implementation */ } } } let is_unix_str = cfg_select! { unix => "unix", _ => "not unix", }; println!("{is_unix_str}"); ``` ## Semantics The expansion of a `cfg_select!` call is the right-hand side of the first `cfg` rule that evaluates to true. This can be roughly expressed using this macro: ```rust macro_rules! cfg_select { ({ $($tt:tt)* }) => {{ $crate::cfg_select! { $($tt)* } }}; (_ => { $($output:tt)* }) => { $($output)* }; ( $cfg:meta => $output:tt $($( $rest:tt )+)? ) => { #[cfg($cfg)] $crate::cfg_select! { _ => $output } $( #[cfg(not($cfg))] $crate::cfg_select! { $($rest)+ } )? } } ``` The actual implementation uses a builtin macro so that `cfg_select!` can be used both in item and expression position. ## Documentation reference PR: - rust-lang/reference#2103 ## Tests The `cfg_select!` macro is already used extensively in the rust compiler codebase. It has several dedicated tests: - [`tests/ui/check-cfg/cfg-select.rs`](https://github.com/rust-lang/rust/blob/main/tests/ui/check-cfg/cfg-select.rs)tests that warnings are emitted when an unexpected `cfg` condition is used. - [`tests/ui/macros/cfg_select.rs`](https://github.com/rust-lang/rust/blob/main/tests/ui/macros/cfg_select.rs) tests that `cfg_select!` has the expected expansion, and tests that the expected syntax is accepted. ## History - rust-lang#115416 - rust-lang#117162 - rust-lang#133720 - rust-lang#135625 - rust-lang#137198 - rust-lang#138993 - rust-lang#138996 - rust-lang#143461 - rust-lang#143941 - rust-lang#145233 - rust-lang#148712 - rust-lang#149380 - rust-lang#149925 # Resolved questions # Unresolved questions The style team has decided on how to format `cfg_select!`, but this formatting has not yet been implemented. See rust-lang#144323. r? @traviscross <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_CONCERN-ISSUE_START --> > [!NOTE] > # Concerns (0 active) > > - ~~[allowing-comma-after-closing-brace](rust-lang#149783 (comment) resolved in [this comment](rust-lang#149783 (comment)) > > *Managed by `@rustbot`—see [help](https://forge.rust-lang.org/triagebot/concern.html) for details.* <!-- TRIAGEBOT_CONCERN-ISSUE_END --> <!-- TRIAGEBOT_END -->

Tracking issue for
cfg_select: #115585When operating on expressions,
cfg_select!can now handle expressionswithout braces. (It still requires braces for other things, such as
items.)
Expand the test coverage and documentation accordingly.
I'm not sure whether deciding to extend
cfg_select!in this way is T-lang or T-libs-api. I've labeled for both, with the request that both teams don't block on each other. :)