Skip to content

reference: clarify #[cfg] section#39374

Merged
bors merged 1 commit into
rust-lang:masterfrom
durka:patch-34
Feb 8, 2017
Merged

reference: clarify #[cfg] section#39374
bors merged 1 commit into
rust-lang:masterfrom
durka:patch-34

Conversation

@durka

@durka durka commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

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

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @steveklabnik

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

@bluss

bluss commented Jan 30, 2017

Copy link
Copy Markdown
Contributor

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

durka commented Jan 30, 2017

Copy link
Copy Markdown
Contributor Author

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

durka commented Jan 30, 2017

Copy link
Copy Markdown
Contributor Author

@bluss updated, what do you think?

Comment thread src/doc/reference.md Outdated

@bluss bluss Jan 30, 2017

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.

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

durka commented Jan 30, 2017 via email

Copy link
Copy Markdown
Contributor Author

@codyps

codyps commented Jan 30, 2017

Copy link
Copy Markdown
Contributor

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

durka commented Feb 7, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

durka: r=me with that change.

@durka

durka commented Feb 7, 2017

Copy link
Copy Markdown
Contributor Author

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

bluss commented Feb 7, 2017

Copy link
Copy Markdown
Contributor

@bors r=steveklabnik

Great!

@bors

bors commented Feb 7, 2017

Copy link
Copy Markdown
Collaborator

📌 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