Skip to content

Conversation

Zalathar
Copy link
Contributor

Follow-up to #132754 (comment).

The name rustc_short_optgroups has always been confusing, because it is unrelated to the distinction between short and long options (i.e. -s vs --long), and instead means something like “the subset of command-line options that are printed by rustc --help without -v”.

So let's merge that function into the main rustc_optgroups, and store the relevant bit of information in a boolean field in RustcOptGroup instead.


This PR also modifies RustcOptGroup to store its various strings directly, instead of inside a boxed apply closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 11, 2024
@jieyouxu
Copy link
Member

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned BoxyUwU Nov 11, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Very nice cleanup! One minor naming nit then r=me.

Comment on lines 1401 to 1408
/// Display name for this option. Normally equal to `long_name`, except for
/// options that don't have a long name.
pub name: &'static str,
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: display_name? We have short_name and long_name below, so having just name feels a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, in the process of looking into this, I remembered that things are actually more complicated.

We do use this as a display name, but it also gets used for other things, such as matching against the option name reported by some getopts errors.

So I'm going to push a better comment, but now I don't like the idea of trying to rename name in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's fine with me.

Comment on lines +1412 to +1419
/// If true, this option should not be printed by `rustc --help`, but
/// should still be printed by `rustc --help -v`.
pub is_verbose_help_only: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Remark: yes thank you, this is so much clearer.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 11, 2024
@Zalathar
Copy link
Contributor Author

Revised comment for name (diff), in light of the fact that it's not just a display name.

@rustbot ready

@rustbot rustbot 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 Nov 11, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Cool

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 11, 2024

📌 Commit 8b4701d has been approved by jieyouxu

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#129627 (Ensure that tail expr receive lifetime extension)
 - rust-lang#130999 (Implement file_lock feature)
 - rust-lang#132873 (handle separate prefixes in clippy rules)
 - rust-lang#132891 (Remove `rustc_session::config::rustc_short_optgroups`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#129627 (Ensure that tail expr receive lifetime extension)
 - rust-lang#130999 (Implement file_lock feature)
 - rust-lang#132873 (handle separate prefixes in clippy rules)
 - rust-lang#132891 (Remove `rustc_session::config::rustc_short_optgroups`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bcd85e5 into rust-lang:master Nov 11, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2024
Rollup merge of rust-lang#132891 - Zalathar:short-opt-groups, r=jieyouxu

Remove `rustc_session::config::rustc_short_optgroups`

Follow-up to rust-lang#132754 (comment).

The name `rustc_short_optgroups` has always been confusing, because it is unrelated to the distinction between short and long options (i.e. `-s` vs `--long`), and instead means something like “the subset of command-line options that are printed by `rustc --help` without `-v`”.

So let's merge that function into the main `rustc_optgroups`, and store the relevant bit of information in a boolean field in `RustcOptGroup` instead.

---

This PR also modifies `RustcOptGroup` to store its various strings directly, instead of inside a boxed `apply` closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.
@Zalathar Zalathar deleted the short-opt-groups branch November 11, 2024 23:47
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Remove `rustc_session::config::rustc_short_optgroups`

Follow-up to rust-lang#132754 (comment).

The name `rustc_short_optgroups` has always been confusing, because it is unrelated to the distinction between short and long options (i.e. `-s` vs `--long`), and instead means something like “the subset of command-line options that are printed by `rustc --help` without `-v`”.

So let's merge that function into the main `rustc_optgroups`, and store the relevant bit of information in a boolean field in `RustcOptGroup` instead.

---

This PR also modifies `RustcOptGroup` to store its various strings directly, instead of inside a boxed `apply` closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#129627 (Ensure that tail expr receive lifetime extension)
 - rust-lang#130999 (Implement file_lock feature)
 - rust-lang#132873 (handle separate prefixes in clippy rules)
 - rust-lang#132891 (Remove `rustc_session::config::rustc_short_optgroups`)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants