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

coverage: Simplify some parts of the coverage span refiner #121019

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Feb 13, 2024

This is another incremental step on my quest to dismantle the coverage span refiner into something more understandable and maintainable.

The biggest change here is splitting up CoverageSpan into several more specific structs. Doing so reveals that most of the places that were using that struct only need a subset of its fields and methods.

We can also get rid of separate tracking of curr_original_span and prev_original_span, by observing that curr.span never actually needs to be mutated, and that we can store prev_original_span directly in the dedicated struct for prev.

@rustbot label +A-code-coverage

Swapping the direction of this merge produces the same results, but means that
we never need to mutate `curr`.
Now that we never mutate `curr.span`, we don't need to store its original span
separately.
@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2024

r? @estebank

rustbot has assigned @estebank.
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 Feb 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Feb 13, 2024
This requires some extra boilerplate, but in exchange it becomes much easier to
see how each field and method is actually used.
Now that `prev` has its own dedicated struct, we can store the original span in
that struct, instead of in a separate field in the refiner.
If we only check for duplicate spans when `prev` is unmodified, we reduce the
number of situations that `update_pending_dups` needs to handle.

This could potentially change the coverage spans we produce in some unknown
corner cases, but none of our current coverage tests indicate any change.
@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 13, 2024

📌 Commit e67db4c has been approved by oli-obk

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 Feb 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#114877 (unstable-book: add quick-edit link)
 - rust-lang#120548 (rustdoc: Fix handling of doc_auto_cfg feature for cfg attributes on glob reexport)
 - rust-lang#120549 (modify alias-relate to also normalize ambiguous opaques)
 - rust-lang#120959 (Remove good path delayed bugs)
 - rust-lang#120978 (match lowering: simplify block creation)
 - rust-lang#121019 (coverage: Simplify some parts of the coverage span refiner)
 - rust-lang#121021 (Extend intra-doc link chapter in the rustdoc book)
 - rust-lang#121031 (RustWrapper: adapt for coverage mapping API changes)

Failed merges:

 - rust-lang#121014 (Remove `force_print_diagnostic`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e36a7f4 into rust-lang:master Feb 13, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2024
Rollup merge of rust-lang#121019 - Zalathar:covspans, r=oli-obk

coverage: Simplify some parts of the coverage span refiner

This is another incremental step on my quest to dismantle the coverage span refiner into something more understandable and maintainable.

The biggest change here is splitting up `CoverageSpan` into several more specific structs. Doing so reveals that most of the places that were using that struct only need a subset of its fields and methods.

We can also get rid of separate tracking of `curr_original_span` and `prev_original_span`, by observing that `curr.span` never actually needs to be mutated, and that we can store `prev_original_span` directly in the dedicated struct for `prev`.

`@rustbot` label +A-code-coverage
@Zalathar Zalathar deleted the covspans branch February 13, 2024 21:43
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 12, 2024
coverage: Replace the old span refiner with a single function

As more and more of the span refiner's functionality has been pulled out into separate early passes, it has finally reached the point where we can remove the rest of the old `SpansRefiner` code, and replace it with a single modestly-sized function.

~~There should be no change to the resulting coverage mappings, as demonstrated by the lack of changes to test output.~~

There is *almost* no change to the resulting coverage mappings. There are some minor changes to `loop` that on inspection appear to be neutral in terms of accuracy, with the old behaviour being a slightly-horrifying implementation detail of the old code, so I think they're acceptable.

Previous work in this direction includes:
- rust-lang#125921
- rust-lang#121019
- rust-lang#119208
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
Rollup merge of rust-lang#126294 - Zalathar:spans-refiner, r=oli-obk

coverage: Replace the old span refiner with a single function

As more and more of the span refiner's functionality has been pulled out into separate early passes, it has finally reached the point where we can remove the rest of the old `SpansRefiner` code, and replace it with a single modestly-sized function.

~~There should be no change to the resulting coverage mappings, as demonstrated by the lack of changes to test output.~~

There is *almost* no change to the resulting coverage mappings. There are some minor changes to `loop` that on inspection appear to be neutral in terms of accuracy, with the old behaviour being a slightly-horrifying implementation detail of the old code, so I think they're acceptable.

Previous work in this direction includes:
- rust-lang#125921
- rust-lang#121019
- rust-lang#119208
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.

5 participants