coverage: Remove the final span-merge pass, and rename is_closure to is_hole
#121433
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
the
A-code-coverage
|
The final merge pass was introduced in #118695, replacing the previous practice of performing a merge on every entry added to the list of refined covspans. |
This step used to be essential, but after other recent changes to span refinement it doesn't appear to be particularly necessary. Removing the final merge step will make it easier to simplify or replace the span refinement code.
Now that we don't perform a final merge pass on refined spans, there's no need to create refined spans for closure/hole spans, so we can just discard them immediately.
When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.)
|
Hmm, thinking about this some more, whatever I replace the span refiner with might want to only have a post-merging step. Perhaps I should experiment with that a bit more before advancing down this particular path. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Since I'm having second thoughts about moving the final span-merge pass, I'll extract the other changes into a new PR. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 23, 2024
coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) `@rustbot` label +A-code-coverage
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 23, 2024
coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) ``@rustbot`` label +A-code-coverage
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2024
Rollup merge of rust-lang#121492 - Zalathar:hole, r=fmease coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) ``@rustbot`` label +A-code-coverage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a combination of semi-related changes that all touch the coverage span-refinement code.
The first significant change is to remove the final span-merging pass that occurs after the main span-refinement code has run. This step used to be essential for getting good coverage mappings, but after other recent changes to span refinement (e.g. #121135 and #121261) it doesn't seem to have much effect any more, so we can simplify the code by removing it.
The other big change is to take code that refers to “closure spans”, and rename it to refer to “hole spans” instead. When checking for the
is_closureflag, we don't specifically care whether the span represents a closure; we just want to know whether it's a span that should be carved out of other spans and then discarded.(Closure spans are currently the only kind of hole span, but in the future might want to add other kinds of hole spans representing nested items or inactive
#[cfg(..)]regions.)@rustbot label +A-code-coverage