Skip to content
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

reference: clarify #[cfg] section #39374

Merged
merged 1 commit into from
Feb 8, 2017
Merged

reference: clarify #[cfg] section #39374

merged 1 commit into from
Feb 8, 2017

Conversation

durka
Copy link
Contributor

@durka durka commented Jan 28, 2017

Closes #39370, but maybe we (or clippy) should add a warning too.

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@bluss
Copy link
Member

bluss commented Jan 30, 2017

It's still misleading, there is no "contains a string". Variables feature="a" and feature="b" can be independently set, so there is no feature and it doesn't contain either a or b.

@durka
Copy link
Contributor Author

durka commented Jan 30, 2017

At least it does collapse whitespace though, so --cfg 'x="y"' and --cfg 'x = "y"' mean the same thing. So it does give the appearance of a property named x that contains the value "y". However, x can take on multiple values at the same time.

@durka
Copy link
Contributor Author

durka commented Jan 30, 2017

@bluss updated, what do you think?


one that is either defined or not
(`#[cfg(foo)]`), and the other that takes on one or more string values
(`#[cfg(bar = "baz")]`).
Copy link
Member

@bluss bluss Jan 30, 2017

Choose a reason for hiding this comment

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

better but why should we say there are two kinds of configuration options, when there is just one? first and second="x" are both just names of cfg vars that can be on or off. Also some old text left in here at the end.

@durka
Copy link
Contributor Author

durka commented Jan 30, 2017 via email

@codyps
Copy link
Contributor

codyps commented Jan 30, 2017

As someone who just recently realized that "first and second="x" are both just names of cfg vars", my opinion is that knowing that is the actual meaning makes them much easier to think about.

Talking about associated keys seems to just confuse things.

@durka
Copy link
Contributor Author

durka commented Feb 7, 2017

OK, new draft:

Configuration options are boolean (on or off) and are named either with a single identifier (e.g. foo) or an identifier and a string (e.g. foo = "bar"; the quotes are required and spaces around the = are unimportant). Note that similarly-named options, such as foo, foo="bar" and foo="baz" may each be set or unset independently.

Configuration options are either provided by the compiler or passed in on the command line using --cfg (e.g. rustc main.rs --cfg foo --cfg 'bar="baz"').

@steveklabnik
Copy link
Member

durka: r=me with that change.

@durka
Copy link
Contributor Author

durka commented Feb 7, 2017

OK, done. I also added a mention of cfg!. Will squash (wish you could do that with the web editor...).

@durka durka changed the title remove lie about #[cfg] from reference reference: clarify #[cfg] section Feb 7, 2017
@bluss
Copy link
Member

bluss commented Feb 7, 2017

@bors r=steveklabnik

Great!

@bors
Copy link
Contributor

bors commented Feb 7, 2017

📌 Commit 620074d has been approved by steveklabnik

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
reference: clarify #[cfg] section

Closes rust-lang#39370, but maybe we (or clippy) should add a warning too.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
reference: clarify #[cfg] section

Closes rust-lang#39370, but maybe we (or clippy) should add a warning too.
bors added a commit that referenced this pull request Feb 8, 2017
Rollup of 13 pull requests

- Successful merges: #38764, #39361, #39372, #39374, #39400, #39426, #39431, #39459, #39482, #39545, #39593, #39620, #39621
- Failed merges:
@bors bors merged commit 620074d into rust-lang:master Feb 8, 2017
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.

6 participants