-
Notifications
You must be signed in to change notification settings - Fork 529
Referenceify conditional compilation #444
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
Referenceify conditional compilation #444
Conversation
src/conditional-compilation.md
Outdated
|
||
Example values: | ||
|
||
* `"x86"`, |
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.
There are some random trailing commas here.
src/conditional-compilation.md
Outdated
> [IDENTIFIER] (`=` ([STRING_LITERAL] | [RAW_STRING_LITERAL]))<sup>?</sup> | ||
> | ||
> _ConfigurationAll_\ | ||
> `all` `(` _ConfigurationPredicate_ (`,` _ConfigurationPredicate_)<sup>\*</sup> `,`<sup>?</sup> `)` |
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.
These don't seem to capture that they can be an empty list. Not sure if that is important, since that is an odd usage.
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 don't have any significant comments. Maybe move the Forms
section before the option list, because you have to scroll all the way to the bottom to see any examples. But that's not critical, either way is fine.
src/conditional-compilation.md
Outdated
> **Note**: The `cfg_attr` can expand to another `cfg_attr`. For example, | ||
> `#[cfg_attr(linux, cfg_attr(feature = "multithreaded", some_other_attribute))` | ||
> is valid. This example would be equivalent to | ||
> `#[cfg_attr(and(linux, feaure ="multithreaded"), some_other_attribute)]`. |
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.
feaure → feature
I don't like having them all the way on the bottom either, and the page is describing things in a bottom-up way. Perhaps we should invert it, and explain the attributes and macro first? |
for targets with 32-bit pointers, this is set to `"32"`. Likewise, it is set | ||
to `"64"` for targets with 64-bit pointers. | ||
|
||
<!-- Are there targets that have a different bit number? --> |
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.
There's a 16 bit platform listed in tier 3 here
Thanks! |
Rendered
Fixes #384
Partial r? @ehuss for grammar additions.
I put a couple questions into comments into the source itself.
I also wonder if making each configuration option set by the compiler its own section like I did is too spacey. I could bring it back down into a list if y'all agree with that.