A big options clean-up by nnethercote · Pull Request #70729 · rust-lang/rust

nnethercote · 2020-04-03T06:20:18Z

Lots of improvements here.

nnethercote · 2020-04-03T06:20:58Z

I recommend reviewing this one commit at a time.

Centril

This looks nice. Here are some things I spotted during review.
However, I'm not too familiar with this part of the codebase, so I'll leave approval to @petrochenkov.

petrochenkov · 2020-04-03T23:22:05Z

This is great, thank you.

After our discussion about -Cembed-bitcode I had the same thought about converting all the flag options (old parse_bool) into boolean options (old parse_opt_bool).

petrochenkov · 2020-04-03T23:22:10Z

I have some reservations regarding all the no-* options which look pretty bad with the new yes/no values due to possible double negation.
With the new scheme I think we need to convert them from no-foo=no/yes to foo=yes/no.

For unstable flag options -Z no-foo we can

Add -Z foo=yes/no
Remove the existing -Z no-foo (possibly later in a separate PR)

For stable flag options -C no-foo we can

Add -C foo=yes/no
Keep the existing -C no-foo for compatibility, without support for yes/no (by introducing parse_flag equivalent to the old parse_bool)

(Or we can just avoid touching the no-* options in this PR at all.)

bors · 2020-04-04T02:53:59Z

☔ The latest upstream changes (presumably #70156) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2020-04-04T06:27:56Z

(Or we can just avoid touching the no-* options in this PR at all.)

I would strongly prefer that.

I agree that --no-foo flags are weird and it would be nice to get rid of them. Having said that, I think that addition/removal/renaming of flags is significant scope creep for this PR, which is about cleaning things up and making things consistent without any significant changes in behaviour.
(Also note that someone writing --no-foo=no is extremely unlikely, because (a) it has been impossible prior to now, and (b) it has no effect unless preceded by --no-foo.)

petrochenkov · 2020-04-04T09:06:15Z

I would strongly prefer that.

Ok.
Could you reintroduce parse_flag then and use it for the no-* options.

nnethercote · 2020-04-04T22:52:38Z

Could you reintroduce parse_flag then and use it for the no-* options.

Advantages of this:

Double-negatives (e.g. --no-foo=no) are not possible.

Disadvantages of this:

--no-foo options would go back to being sticky, i.e. not overridable.
We would lose the new consistency that all options can take values.
We would require an extra parse_* function.
We would have to remove the "Make option type descriptions non-optional" commit, which was a nice simplification.

In my opinion, the disadvantages outweigh the advantages, particularly given that the number of --no-foo options is likely to drop in the future.

petrochenkov · 2020-04-04T22:59:25Z

@nnethercote
Looks like we misinterpreted each other then?

In "avoid touching the no-* options in this PR at all" I meant not touching them compared to the status quo (before this PR), not compared to this PR as is.
You were also talking about "making things consistent without any significant changes" which I also interpreted as support for the status quo.

petrochenkov · 2020-04-04T23:06:41Z

Double-negatives (e.g. --no-foo=no) are not possible.

--no-foo=yes, which is equally confusing, also becomes impossible.

--no-foo options would go back to being sticky, i.e. not overridable.

I'll fix this and introduce the corresponding --foo options once the option field redirection from #70665 lands. (That PR will be less about cleanup, and more about significant changes.)

We would lose the new consistency that all options can take values.

Temporarily, see the previous comment.

We would require an extra parse_* function.

Certainly not a big deal.

We would have to remove the "Make option type descriptions non-optional" commit

Why can't we come up with some description for parse_flag and keep it? It's indeed a nice simplification.

nnethercote · 2020-04-04T23:09:40Z

Sorry for the confusion. I like the PR code as is. It deliberately introduces the minor functional change that all boolean options, including --no-foo ones, can take a value. I call this "minor" because it doesn't require any rustc user to change how they use the compiler. (Though they can, if they want to.)

In contrast, I consider renaming flags or removing flags "non-minor" because rustc users might have to change how they use the compiler.

nnethercote · 2020-04-05T07:15:05Z

Hmm, -C no-redzone complicates things. It's an existing no option that already takes a value.

A problem with the parse_flag suggestion: if we end up with -C no-foo and -C foo=<bool>, then any use site has to check for both cg.no_foo and cg.foo, which is error-prone.

Here's another possibility that avoids that problem, and also gets us away from the use of no more quickly.

Switch all the --no-foo options to --foo=<bool>...
...but give them a marking that allows --no-foo to be allowed as a synonym for --foo=no.
In the documentation, use the --foo= form, but mention that --no-foo is a deprecated alternative.
Later on (though perhaps quite soon), remove the marking for all the current -Z no-foo options.
We'll probably need to keep the marking for the current -C no-foo options for some time, or forever.

eddyb · 2020-04-05T09:11:15Z

cc @rust-lang/compiler

petrochenkov · 2020-04-05T13:35:14Z

@nnethercote

A problem with the parse_flag suggestion: if we end up with -C no-foo and -C foo=<bool>, then any use site has to check for both cg.no_foo and cg.foo, which is error-prone.

I wanted to use the same method that is used in #70665 for merging -C link-arg into -C link-args to merge -C no-foo into -C foo and avoid the mentioned error prone situation.
The macro_rules! redirect_field table serves as the marking in that case, but perhaps it can be integrated with the options! DSL in a nicer way.

nnethercote · 2020-04-06T00:34:43Z

I added a commit with a parse_no_flag function that reverts the changes to the no- flags. This commit is highly prone to conflicts and that seemed like the fastest way to get it landed.

bors · 2020-04-06T02:34:56Z

☔ The latest upstream changes (presumably #70826) made this pull request unmergeable. Please resolve the merge conflicts.

- No trailing '.' chars. - Use a lower-case letter at the start.

They now all accept yes/no/y/n/on/off values. (Previously only some of them did.) This commit also makes `parse_bool` and `parse_opt_bool` more concise and readable, and adds some helpful comments to some functions.

Put identical ones next to each other, and avoid duplicated strings.

Currently, if you give a bogus value like `-Zsanitizer-memory-track-origins=99` you get this incorrect error: ``` error: debugging option `sanitizer-memory-track-origins` takes no value ``` This commit fixes it so it gives this instead: ``` error: incorrect value `99` for debugging option `sanitizer-memory-track-origins` - 0, 1, or 2 was expected ``` The commit also makes `parse_sanitizer_memory_track_origins` more readable.

Don't set `slot` on failure, like all the other `parse_*` functions.

Because all options now can take a value. This simplifies some code quite a bit.

This lets us specify the default at the options declaration point, instead of using `.unwrap(default)` or `None | Some(default)` at some use point far away. It also makes the code more concise.

For all `-C` and `-Z` options that have them. The commit also rewords a few options to make them clearer, mostly by avoiding the word "don't". It also removes the listed default for `-Cinline-threshold`, which is incorrect -- that option doesn't have a static default.

This commit: - Adds "following values" indicators for all the options that are missing them. - Tweaks some wording and punctuation for consistency. - Rewords some things for clarity. - Removes the `no-integrated-as` entry, because that option was removed in rust-lang#70345.

With the exception of `-C no-redzone`, because that could take a value before this PR. This partially undoes one of the earlier commits in this PR, which added the ability to take a value to all boolean options that lacked it. The help output for these options looks like this: ``` -C no-vectorize-slp=val -- disable LLVM's SLP vectorization pass ``` The "=val" part is a lie, but hopefully this will be fixed in the future.

nnethercote · 2020-04-19T11:02:18Z

@bors p=1 because this PR is conflict-prone and other PRs will soon be depending on it.

rfcbot · 2020-04-20T09:36:50Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

petrochenkov · 2020-04-20T09:55:48Z

@bors r+

bors · 2020-04-20T09:55:49Z

📌 Commit 3e3fd73 has been approved by petrochenkov

bors · 2020-04-20T12:30:51Z

⌛ Testing commit 3e3fd73 with merge 8ce3f84...

bors · 2020-04-20T15:43:18Z

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 8ce3f84 to master...

rust-highfive assigned Centril Apr 3, 2020

petrochenkov self-assigned this Apr 3, 2020

fbstj reviewed Apr 3, 2020
View reviewed changes

Comment threadsrc/librustc_session/options.rs Outdated

Centril reviewed Apr 3, 2020
View reviewed changes

Comment threadsrc/librustc_session/options.rs Outdated
Comment threadsrc/librustc_session/options.rs Outdated
Comment threadsrc/doc/rustc/src/codegen-options/index.md Outdated

Centril removed their assignment Apr 3, 2020

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2020

petrochenkov mentioned this pull request Apr 3, 2020
Do not lose or reorder user-provided linker arguments #70665
Merged

nnethercote force-pushed the a-big-options-clean-up branch from bcf2973 to 07a6a56Compare April 6, 2020 00:33

nnethercote force-pushed the a-big-options-clean-up branch from 07a6a56 to 80a640dCompare April 6, 2020 03:14

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020

nnethercote added 10 commits April 19, 2020 19:53

Make some option descriptions fit the usual pattern.
2109464
- No trailing '.' chars. - Use a lower-case letter at the start.

Allow all boolean options to take values.
94b93d3
They now all accept yes/no/y/n/on/off values. (Previously only some of them did.) This commit also makes `parse_bool` and `parse_opt_bool` more concise and readable, and adds some helpful comments to some functions.

Clean up the list of parser descriptions.
0b92969
Put identical ones next to each other, and avoid duplicated strings.

Tweak parse_opt_uint.
3093018
Don't set `slot` on failure, like all the other `parse_*` functions.

Make option type descriptions non-optional.
b9bcddc
Because all options now can take a value. This simplifies some code quite a bit.

Replace uses of parse_opt_* with parse_* where possible.
58217bc
This lets us specify the default at the options declaration point, instead of using `.unwrap(default)` or `None | Some(default)` at some use point far away. It also makes the code more concise.

nnethercote force-pushed the a-big-options-clean-up branch from 066fa09 to 3e3fd73Compare April 19, 2020 10:05

nnethercote mentioned this pull request Apr 19, 2020
Add -Cbitcode-in-rlib. #71323
Merged

rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Apr 20, 2020

rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 20, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team labels Apr 20, 2020

bors mentioned this pull request Apr 20, 2020
attempt to recover perf by removing exports_all_green#71267
Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2020

bors merged commit 8ce3f84 into rust-lang:masterApr 20, 2020

This was referenced Apr 20, 2020
Remove -Z no-landing-pads flag #70175
Merged
Define UB in float-to-int casts to saturate #71269
Merged

nnethercote deleted the a-big-options-clean-up branch April 20, 2020 22:12

ehuss mentioned this pull request May 27, 2020
Add some guidelines on the CLI. rust-lang/rustc-dev-guide#717
Merged

Uh oh!

Uh oh!

Conversation

nnethercote commented Apr 3, 2020

Uh oh!

nnethercote commented Apr 3, 2020

Uh oh!

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Apr 3, 2020

Uh oh!

petrochenkov commented Apr 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Apr 4, 2020

Uh oh!

nnethercote commented Apr 4, 2020

Uh oh!

petrochenkov commented Apr 4, 2020

Uh oh!

nnethercote commented Apr 4, 2020

Uh oh!

petrochenkov commented Apr 4, 2020

Uh oh!

petrochenkov commented Apr 4, 2020

Uh oh!

nnethercote commented Apr 4, 2020

Uh oh!

nnethercote commented Apr 5, 2020

Uh oh!

eddyb commented Apr 5, 2020

Uh oh!

petrochenkov commented Apr 5, 2020

Uh oh!

nnethercote commented Apr 6, 2020

Uh oh!

bors commented Apr 6, 2020

Uh oh!

nnethercote commented Apr 19, 2020

Uh oh!

rfcbot commented Apr 20, 2020

Uh oh!

petrochenkov commented Apr 20, 2020

Uh oh!

bors commented Apr 20, 2020

Uh oh!

bors commented Apr 20, 2020

Uh oh!

bors commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

petrochenkov commented Apr 3, 2020•
edited
Loading