Skip to content

replace cfg_if with cfg_select#1224

Merged
tgross35 merged 1 commit into
rust-lang:mainfrom
hkBst:replace-cfg-if
Jun 30, 2026
Merged

replace cfg_if with cfg_select#1224
tgross35 merged 1 commit into
rust-lang:mainfrom
hkBst:replace-cfg-if

Conversation

@hkBst

@hkBst hkBst commented Jun 24, 2026

Copy link
Copy Markdown
Member

Now that cfg_select is stable, we can replace cfg_if. One difference is that cfg_select needs one branch to match. Therefore an empty default branch is sometimes needed.

EDIT: now using a local impl of cfg_select (thanks @folkertdev [1]) so as not to have to increase msrv.

[1]:#t-libs > compiler-builtins msrv policy? @ 💬

@tgross35 tgross35 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the interest, but I don't think there is any need to do this. We can switch to cfg_select as minimum versions across the repo are raised, but the status quo works sufficiently well that there's no reason for cfg_select to be the driving factor.

View changes since this review

@rust-cloud-vms
rust-cloud-vms Bot force-pushed the replace-cfg-if branch 4 times, most recently from 415dc77 to fcf6ccc Compare June 25, 2026 08:05
@hkBst

hkBst commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

@tgross35 this now no longer needs an msrv bump due to a local impl, but should make switching to the real cfg_select trivial when msrv reaches 1.95+. What do you think?

@tgross35

Copy link
Copy Markdown
Contributor

This looks doable, thanks! I'm going to hold off until the patch for the formatting bug lands.

@folkertdev

Copy link
Copy Markdown
Contributor

What formatting bug? rust-lang/rust#158371 is specific to the builtin cfg_select and should not affect a macro_rules! macro? or if it is broken, that fix won't fix it.

@tgross35

Copy link
Copy Markdown
Contributor

I thought this used the real cfg_select in some cases, but I guess that isn't the case.

@tgross35 tgross35 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One request then LGTM

View changes since this review

Comment on lines -1 to +3
/// `libm` cannot have dependencies, so this is vendored directly from the `cfg-if` crate
/// (with some comments stripped for compactness).
macro_rules! cfg_if {
(
if #[cfg( $($i_meta:tt)+ )] { $( $i_tokens:tt )* }
$(
else if #[cfg( $($ei_meta:tt)+ )] { $( $ei_tokens:tt )* }
)*
$(
else { $( $e_tokens:tt )* }
)?
) => {
cfg_if! {
@__items () ;
(( $($i_meta)+ ) ( $( $i_tokens )* )),
$(
(( $($ei_meta)+ ) ( $( $ei_tokens )* )),
)*
$(
(() ( $( $e_tokens )* )),
)?
}
#[macro_export]
macro_rules! cfg_select {
({ $($tt:tt)* }) => {{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a note about why we have this? Can just be a // FIXME(msrv):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done!

@rustbot

rustbot commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@tgross35 tgross35 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tgross35
tgross35 merged commit 14155c5 into rust-lang:main Jun 30, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants