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

Remove -Zdump-mir-spanview #119566

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Remove -Zdump-mir-spanview #119566

merged 1 commit into from
Jan 5, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jan 4, 2024

The -Zdump-mir-spanview flag was added back in #76074, as a development/debugging aid for the initial work on what would eventually become -Cinstrument-coverage. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to triage and remove unnecessary -Z flags (Zulip), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on tests/coverage/closure.rs). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.


@rustbot label +A-code-coverage

@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2024

r? @TaKO8Ki

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

@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. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Jan 4, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 4, 2024

While I was in the area I also noticed some opportunities to simplify pretty.rs by inlining create_dump_file_with_basename and dump_file_basename, but I decided to keep this PR clean and pursue those changes later in a separate PR.

Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

good to see all this code gone. unfortunate that it was not useful with helping/guiding your recent work to improve all that span handling.

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Tried this out once and while it was pretty cute, if it's not useful it should be removed.

@Noratrieb
Copy link
Member

@bors r=Swatinem,Nilstrieb

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit af32054 has been approved by Swatinem,Nilstrieb

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 Jan 4, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 4, 2024

I might end up having to rebase this after #119531 lands.

@compiler-errors
Copy link
Member

@bors rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

`@rustbot` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

``@rustbot`` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117556 (Disallow reference to `static mut` and adding `static_mut_ref` lint)
 - rust-lang#119354 (Make `negative_bounds` internal & fix some of its issues)
 - rust-lang#119420 (Handle ForeignItem as TAIT scope.)
 - rust-lang#119506 (Use `resolutions(()).effective_visiblities` to avoid cycle errors in `report_object_error`)
 - rust-lang#119566 (Remove `-Zdump-mir-spanview`)
 - rust-lang#119567 (Remove `-Zreport-delayed-bugs`.)
 - rust-lang#119577 (Migrate memory overlap check from validator to lint)
 - rust-lang#119586 ([rustdoc] Fix invalid handling for static method calls in jump to definition feature)
 - rust-lang#119588 (Move `i586-unknown-netbsd` from tier 2 to tier 3 platform support table)

r? `@ghost`
`@rustbot` modify labels: rollup
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 5, 2024
…em,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

```@rustbot``` label +A-code-coverage
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
…llaumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#119354 (Make `negative_bounds` internal & fix some of its issues)
 - rust-lang#119414 (bootstrap: Move -Clto= setting from Rustc::run to rustc_cargo)
 - rust-lang#119420 (Handle ForeignItem as TAIT scope.)
 - rust-lang#119506 (Use `resolutions(()).effective_visiblities` to avoid cycle errors in `report_object_error`)
 - rust-lang#119538 (Cleanup error handlers: round 5)
 - rust-lang#119566 (Remove `-Zdump-mir-spanview`)
 - rust-lang#119567 (Remove `-Zreport-delayed-bugs`.)
 - rust-lang#119577 (Migrate memory overlap check from validator to lint)
 - rust-lang#119586 ([rustdoc] Fix invalid handling for static method calls in jump to definition feature)
 - rust-lang#119588 (Move `i586-unknown-netbsd` from tier 2 to tier 3 platform support table)

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

Rollup of 10 pull requests

Successful merges:

 - rust-lang#119034 (Allow coverage tests to ignore test modes, and to enable color in coverage reports)
 - rust-lang#119148 (Tweak suggestions for bare trait used as a type)
 - rust-lang#119538 (Cleanup error handlers: round 5)
 - rust-lang#119566 (Remove `-Zdump-mir-spanview`)
 - rust-lang#119567 (Remove `-Zreport-delayed-bugs`.)
 - rust-lang#119577 (Migrate memory overlap check from validator to lint)
 - rust-lang#119583 (Make `intrinsics::assume` const stable)
 - rust-lang#119586 ([rustdoc] Fix invalid handling for static method calls in jump to definition feature)
 - rust-lang#119588 (Move `i586-unknown-netbsd` from tier 2 to tier 3 platform support table)
 - rust-lang#119601 (`Emitter` cleanups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e74a0cd into rust-lang:master Jan 5, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
Rollup merge of rust-lang#119566 - Zalathar:remove-spanview, r=Swatinem,Nilstrieb

Remove `-Zdump-mir-spanview`

The `-Zdump-mir-spanview` flag was added back in rust-lang#76074, as a development/debugging aid for the initial work on what would eventually become `-Cinstrument-coverage`. It causes the compiler to emit an HTML file containing a function's source code, with various spans highlighted based on the contents of MIR.

When the suggestion was made to [triage and remove unnecessary `-Z` flags (Zulip)](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Z.60.20option.20triage), I noted that this flag could potentially be worth removing, but I wanted to keep it around to see whether I found it useful for my own coverage work.

But when I actually tried to use it, I ran into various issues (e.g. it crashes on `tests/coverage/closure.rs`). If I can't trust it to work properly without a full overhaul, then instead of diving down a rabbit hole of trying to fix arcane span-handling bugs, it seems better to just remove this obscure old code entirely.

---

````@rustbot```` label +A-code-coverage
@Zalathar Zalathar deleted the remove-spanview branch January 5, 2024 21:20
Zalathar added a commit to Zalathar/rustc-dev-guide that referenced this pull request Jan 16, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Jan 16, 2024
HTML files were needed for testing `-Z dump-mir-spanview`, but that flag has
been removed by rust-lang#119566.
tshepang pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Jan 16, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2024
Don't allow `.html` files in `tests/mir-opt/`

HTML files were needed for testing `-Z dump-mir-spanview`, but that flag has been removed by rust-lang#119566.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2024
Rollup merge of rust-lang#120018 - Zalathar:mir-opt-html, r=Nilstrieb

Don't allow `.html` files in `tests/mir-opt/`

HTML files were needed for testing `-Z dump-mir-spanview`, but that flag has been removed by rust-lang#119566.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
Don't create a separate "basename" when naming and opening a MIR dump file

These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title.

That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path.

This PR therefore restores things to (roughly) how they were before rust-lang#77080.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
Don't create a separate "basename" when naming and opening a MIR dump file

These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title.

That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path.

This PR therefore restores things to (roughly) how they were before rust-lang#77080.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
Don't create a separate "basename" when naming and opening a MIR dump file

These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title.

That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path.

This PR therefore restores things to (roughly) how they were before rust-lang#77080.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
Rollup merge of rust-lang#120038 - Zalathar:dump-path, r=WaffleLapkin

Don't create a separate "basename" when naming and opening a MIR dump file

These functions were split up by rust-lang#77080, in order to support passing the dump file's “basename” (filename without extension) to the implementation of `-Zdump-mir-spanview`, so that it could be used as a page title.

That flag has since been removed (rust-lang#119566), so now there's no particular reason for this code to handle the basename separately from the filename or full path.

This PR therefore restores things to (roughly) how they were before rust-lang#77080.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) 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.

7 participants