-
Notifications
You must be signed in to change notification settings - Fork 571
cfg_select! macro
#2103
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
base: master
Are you sure you want to change the base?
cfg_select! macro
#2103
Conversation
30ce14a to
a93da68
Compare
11dedff to
6cbeb12
Compare
| r[cfg.cfg_select] | ||
| ### The `cfg_select` macro | ||
|
|
||
| The built-in `cfg_select` macro expands to the right-hand side of the first configuration predicate that evaluates to `true`. | ||
|
|
||
| For example: | ||
|
|
||
| ```rust | ||
| cfg_select! { | ||
| unix => { | ||
| fn foo() { /* unix specific functionality */ } | ||
| } | ||
| target_pointer_width = "32" => { | ||
| fn foo() { /* non-unix, 32-bit functionality */ } | ||
| } | ||
| _ => { | ||
| fn foo() { /* fallback implementation */ } | ||
| } | ||
| } | ||
| ``` | ||
| The `cfg_select` macro can also be used in expression position: | ||
|
|
||
| ```rust | ||
| let is_unix_str = cfg_select! { | ||
| unix => "unix", | ||
| _ => "not unix", | ||
| }; | ||
| ``` | ||
|
|
||
| A `_` can be used to write a configuration predicate that always evaluates to `true`. |
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.
Thanks for the Reference PR. It'd be worth filling in the details you provided in rust-lang/rust#149783 (comment), e.g. with the grammar, the removal of one level of braces, the compile error on fallthrough, etc.
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.
This can discuss as well how the bodies of each branch must be able to parse.
folkertdev
left a comment
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 think all the raw information is here now, but it probably needs some editing/clarification.
Also should the lint on branches that come after a wildcard be mentioned?
src/conditional-compilation.md
Outdated
| r[cfg.cfg_select.general] | ||
| The built-in `cfg_select` macro expands to the `TokenTree` on the right-hand side of the first configuration predicate that evaluates to `true`. |
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.
is it clear enough from the grammer and the language here that the { /* ... */ } are dropped?
| r[cfg.cfg_select.positions] | ||
| The `cfg_select!` macro is accepted in the following macro expansion positions | ||
|
|
||
| - items | ||
| - statements | ||
| - expression | ||
| - impl items | ||
| - trait impl items | ||
| - trait items | ||
| - foreign items |
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 can't find the positions in which macro expansion is allowed in the reference, actually.
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.
The list of macro invocation sites is listed in https://doc.rust-lang.org/nightly/reference/macros.html#r-macro.invocation.intro. I don't think this needs to duplicate the list.
We haven't really gotten into documenting built-in attributes, so there isn't a well-trodden path here. I think it would be fine to say that it may specified in any position where macro invocations are allowed.
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.
Also, did you intend to exclude patterns and types in this list?
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 suspect that the list was accurate at the time, but it no longer is and should now include patterns and types.
ehuss
left a comment
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.
Also should the lint on branches that come after a wildcard be mentioned?
As for lints, we normally don't document those. However, in this situation, it seems like it would be fine to have a > [!NOTE] block that mentions the lint (in whichever way rust-lang/rust#149960 ends up being).
| ```grammar,configuration | ||
| CfgSelect -> | ||
| cfg_select! `{` CfgSelectBranch* `}` | ||
| CfgSelectConfigurationPredicate -> | ||
| ConfigurationPredicate | `_` | ||
| CfgSelectBranch -> | ||
| CfgSelectConfigurationPredicate `=>` `{` TokenTree `}` | ||
| | CfgSelectConfigurationPredicate `=>` TokenTree `,` | ||
| ``` |
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.
This grammar doesn't quite look correct to me.
Some comments:
- We don't have a precedent for documenting macro inputs. I don't think we can express it like
`cfg_select!` `{` … `}`because the braces can also be()or[](also!is a separate token), and the macro name can be renamed. I would suggest just documenting what the input to the macro is, and not the surrounding invocation syntax. - This doesn't handle the difference in behavior of the last branch.
- This doesn't specify that the non-braced value needs to be an expression of a certain kind.
- This doesn't handle the optional
,behavior.
I'm thinking the grammar would be something closer to:
@root CfgSelect ->
CfgSelectBranch*
LastCfgSelectBranch?
CfgSelectBranch ->
CfgSelectConfigurationPredicate `=>` `{` TokenTree `}`
| CfgSelectConfigurationPredicate `=>` ExpressionWithBlockX `,`?
| CfgSelectConfigurationPredicate `=>` ExpressionWithoutBlockX `,`
ExpressionWithBlockX ->
BlockExpression
| ConstBlockExpression
| UnsafeBlockExpression
| LoopExpression
| IfExpression
| MatchExpression
ExpressionWithoutBlockX ->
LiteralExpression
| PathExpression
| OperatorExpression
| GroupedExpression
| ArrayExpression
| AwaitExpression
| IndexExpression
| TupleExpression
| TupleIndexingExpression
| StructExpression
| CallExpression
| MethodCallExpression
| FieldExpression
| ClosureExpression
| AsyncBlockExpression
| ContinueExpression
| BreakExpression
| RangeExpression
| ReturnExpression
| UnderscoreExpression
| MacroInvocation
LastCfgSelectBranch ->
CfgSelectConfigurationPredicate => ExpressionX `,`?
ExpressionX -> ExpressionWithBlockX | ExpressionWithoutBlockX
CfgSelectConfigurationPredicate ->
ConfigurationPredicate | `_`
Where the X expressions are from the Expression grammar, but without the outer attributes. If this is correct, we'll need to rework Expression to support that.
Does that make sense?
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.
Though looking again, my suggestion above won't work because we are moving towards a grammar that does not have infinite lookahead for disambiguation. So the LastCfgSelectBranch won't work. I offhand can't think of a way to actually express that...
(Maybe lookahead is required?)
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.
Pushed something that might be closer.
|
What does this line do when checking for a closebrace? I would expect that:
That is, I would expect that |
646c927 to
ef0efc4
Compare
|
This PR was rebased onto a different master 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. |
src/conditional-compilation.md
Outdated
| ExpressionWithBlockNoAttrs `,`? CfgSelectArms? | ||
| | ExpressionWithoutBlockNoAttrs ( `,` CfgSelectArms? )? |
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 think this needs to include { TokenTree }. Otherwise, this wouldn't match other non-expression things like patterns, types, etc.
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.
Yes... updated to:
CfgSelectArms ->
CfgSelectConfigurationPredicate `=>`
(
`{` ^ TokenTree `}` CfgSelectArms?
| ExpressionWithBlockNoAttrs `,`? CfgSelectArms?
| ExpressionWithoutBlockNoAttrs ( `,` CfgSelectArms? )?
)It needs the cut because if it takes the first branch but then there's a comma after the closing brace, we can't have it backtrack and succeed on the second branch. (Negative lookahead in the second branch would also work for this.)
@folkertdev: Is there a reason for not allowing an optional comma after the closing brace?
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.
@ehuss: Do you think it's possible to be exhaustive here? I.e., rather than TokenTree, could we have some more-semantic production that comprised patterns, types, etc., or does not work?
Macros can be renamed when imported and can delimit their bodies with different tokens, so we don't want to document the outer part of the call; we just want to document the input to the macro. Doing this for `cfg_select!` is a bit tricky since an expression with a block doesn't need to be followed by a comma while an expression without a block does unless it's the last one. We don't want to handle this the way that the `match` grammar does, currently, since we want to move toward finite lookahead. So, instead, we handle this with right recursion in `CfgSelectArms`. Unlike `match`, outer attributes aren't allowed on the RHS expressions. To handle this, we need to refactor `ExpressionWithoutBlock` and `ExpressionWithBlock`. We also document that the outer braces are removed during expansion when the payload is a block expression. (We called it the arm "payload" after running out of other options.)
ef0efc4 to
daa84b9
Compare
Stabilization report: rust-lang/rust#149783