-
Notifications
You must be signed in to change notification settings - Fork 533
Document #[cfg(version(...))] #1828
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?
Conversation
src/conditional-compilation.md
Outdated
r[cfg.predicate.version] | ||
* `version()` with a version number inside. It is true if the language version | ||
the compiler targets is higher or equal to the contained version number. | ||
It is false otherwise. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Can you make sure to not wrap lines when adding new content?
src/conditional-compilation.md
Outdated
r[cfg.version] | ||
## The `version()` predicate |
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.
It seems a little confusing to have both this and cfg.predicate.version
. I'm wondering if it would make sense to extend the "option" grammar to include version(...)
, and just remove cfg.predicate.version
? Then version()
would just be another option shown in the list here.
And the section header should probably be third level, and match the style of the other options.
r[cfg.version] | |
## The `version()` predicate | |
r[cfg.version] | |
### `version()` |
src/conditional-compilation.md
Outdated
of the language the compiler targets. Usually the compiler version and | ||
language version match. So compiler version `1.50.0` targets language | ||
`1.50.0`. |
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.
Usually the reference focuses on the language and not specifics about the compiler or specific implementations. The text here seems a little confusing in that regard. I would probably just strike this content.
of the language the compiler targets. Usually the compiler version and | |
language version match. So compiler version `1.50.0` targets language | |
`1.50.0`. | |
of the language the compiler targets. |
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 it might be beneficial to discuss a bit how this would work in alternative implementations like mrustc
/ gccrs
? Though maybe "the language the compiler targets" is enough for that, unsure how I'd state it differently.
src/conditional-compilation.md
Outdated
|
||
r[cfg.version.format] | ||
In order for it to be considered of valid format, the version number has to | ||
follow either the `"a.b.c"` scheme or the `"a.b"` scheme, where `a,b,c` are |
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.
follow either the `"a.b.c"` scheme or the `"a.b"` scheme, where `a,b,c` are | |
follow either the `"a.b.c"` scheme or the `"a.b"` scheme, where `a`, `b`, and `c` are |
src/conditional-compilation.md
Outdated
r[cfg.version.format] | ||
In order for it to be considered of valid format, the version number has to | ||
follow either the `"a.b.c"` scheme or the `"a.b"` scheme, where `a,b,c` are | ||
decimal integers between `0` and `65535`, inclusively. Semantically, assume `c` |
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.
Question: Does this detail matter in the reference? Isn't it enough to say "these should be integers", and leave the range to the compiler implementation?
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.
@traviscross and I discussed this a little bit and felt like this probably should be specified? For example, if you say cfg(version("1.81.65535"))
, that will be true, but cfg(version("1.81.65536"))
will be false. Since the magnitude of what gets parsed seems to matter, it might be good to be clear about that.
This is a bit extreme, though. Even if an implementation used u8, it's still 19 years before 1.256 (assuming we stick with 6 week releases), and seems silly for someone to put an unnecessarily large number there.
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.
hmm yeah good point. For future compatibility we will warn on unknown version literal formats, which includes 1.81.65536
. We should probably word it in a way that the unknown formats are not guaranteed to evaluate to false.
a3cfd48
to
c94399f
Compare
src/conditional-compilation.md
Outdated
@@ -23,6 +24,15 @@ ConfigurationAny -> | |||
ConfigurationNot -> | |||
`not` `(` ConfigurationPredicate `)` | |||
ConfigurationVersion -> | |||
`version` `(` `"` ConfigurationVersionLiteral `"` `)` |
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.
Shouldn't this be:
`version` `(` `"` ConfigurationVersionLiteral `"` `)` | |
`version` `(` ( STRING_LITERAL | RAW_STRING_LITERAL ) `)` |
and assuming it is, I would just delete the other grammar productions and describe the format of the string in cfg.predicate.version
in plain English. Or, we can keep ConfigurationVersionLiteral
if you want, but the text will still need to be updated to say that the string needs to conform to the format described by ConfigurationVersionLiteral
.
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.
yeah I think STRING_LITERAL | RAW_STRING_LITERAL
is fine.
This works
#[cfg(version(r"1.0"))] fn foo() {}
foo();
note that syntactically, we don't require string literals here, i.e. this doesn't give an error:
#[cfg(any())]
#[cfg(version(hello(world)))] fn foo() {}
#[cfg(any())]
#[cfg(version(123))] fn foo() {}
probably the same for all cfg syntaxes.
r[cfg.predicate.version] | ||
* `version()` with a version number inside. It is true if the language version |
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.
Can we merge cfg.version
into this section so that it isn't described in two places?
src/conditional-compilation.md
Outdated
r[cfg.version.format] | ||
In order for it to be considered of valid format, the version number has to | ||
follow either the `"a.b.c"` scheme or the `"a.b"` scheme, where `a,b,c` are | ||
decimal integers between `0` and `65535`, inclusively. Semantically, assume `c` |
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.
@traviscross and I discussed this a little bit and felt like this probably should be specified? For example, if you say cfg(version("1.81.65535"))
, that will be true, but cfg(version("1.81.65536"))
will be false. Since the magnitude of what gets parsed seems to matter, it might be good to be clear about that.
This is a bit extreme, though. Even if an implementation used u8, it's still 19 years before 1.256 (assuming we stick with 6 week releases), and seems silly for someone to put an unnecessarily large number there.
Document
#[cfg(version(...))]
as it is to be stabilized.Tracking issue: rust-lang/rust#64796
RFC: rust-lang/rfcs#2523
Stabilization PR: rust-lang/rust#141137
Earlier documentation PR filed for the first attempt: #981