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

-Z location-detail: provide option to disable all location details #99620

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

hudson-ayers
Copy link
Contributor

As reported here, when I first implemented the -Z location-detail flag there was a bug, where passing an empty list was not correctly supported, and instead rejected by the compiler. This PR fixes that such that passing an empty list results in no location details being tracked, as originally specified in rust-lang/rfcs#2091 .

This PR also adds a test case to verify that this option continues to work as intended.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 22, 2022
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022
@GuillaumeGomez
Copy link
Member

I'm not the right reviewer for this change.

r? @davidtwco

@Aaron1011
Copy link
Member

Personally, I think it might be more intuitive to have an explicit none or no option (which would be incompatible with all other options).

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I agree with @Aaron1011, I'd prefer to see this with a none or no option than accepting an empty list. As far as I can gather from skimming the parse functions, we don't have any other options that accept an empty list, but we do have some that accept none/no.

@davidtwco davidtwco 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 Jul 25, 2022
@hudson-ayers hudson-ayers force-pushed the fix-location-detail branch 2 times, most recently from 0f78281 to 455e470 Compare July 26, 2022 17:58
@hudson-ayers
Copy link
Contributor Author

Updated to support none instead of an empty list

@klensy
Copy link
Contributor

klensy commented Jul 26, 2022

Should be fixed also there (but i don't see where it used)

pub const parse_location_detail: &str =
"comma separated list of location details to track: `file`, `line`, or `column`";

and there
location_detail: LocationDetail = (LocationDetail::all(), parse_location_detail, [TRACKED],
"comma separated list of location details to be tracked when using caller_location \
valid options are `file`, `line`, and `column` (default: all)"),

Should all (or added as valid one) option be reworded? As if all passed as argument, it isn't accepted.

@hudson-ayers hudson-ayers force-pushed the fix-location-detail branch 2 times, most recently from 9956b6f to 6e89c4c Compare July 28, 2022 15:14
@hudson-ayers hudson-ayers changed the title -Z location-detail: disable all location details when passed empty list -Z location-detail: provide option to disable all location details Jul 28, 2022
@hudson-ayers
Copy link
Contributor Author

Should be fixed also there (but i don't see where it used)

pub const parse_location_detail: &str =
"comma separated list of location details to track: `file`, `line`, or `column`";

Fixed. And I agree it is confusing to see where this constant is used -- the key is that the options!() macro has an implicit requirement that you pass as $parse a name which is both the name of the function that parses the option in the parse module and also the name of the options descriptor string in the desc module. So this is used here: https://github.com/rust-lang/rust/blob/6e89c4cfb1c76a56ee70d19af6ce1070d4bd7cec/compiler/rustc_session/src/options.rs#L1364

and there

location_detail: LocationDetail = (LocationDetail::all(), parse_location_detail, [TRACKED],
"comma separated list of location details to be tracked when using caller_location \
valid options are `file`, `line`, and `column` (default: all)"),

Should all (or added as valid one) option be reworded? As if all passed as argument, it isn't accepted.

I removed any mention of all and specified that the default is file,line,column.

Prior to this fix, `-Z location-detail` provided no mechanism for
disabling all location details. This commit also adds a test case
to verify that this option continues to work as intended, and
clarifies the documentation of this option.
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2022

📌 Commit 6dea21a has been approved by davidtwco

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 29, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 1, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#99519 (Remove implicit names and values from `--cfg` in `--check-cfg`)
 - rust-lang#99620 (`-Z location-detail`: provide option to disable all location details)
 - rust-lang#99932 (Fix unwinding on certain platforms when debug assertions are enabled)
 - rust-lang#99973 (Layout things)
 - rust-lang#99980 (Remove more Clean trait implementations)
 - rust-lang#99984 (Fix compat.rs for `cfg(miri)`)
 - rust-lang#99986 (Add wrap suggestions for record variants)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9cc06eb into rust-lang:master Aug 1, 2022
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.

8 participants