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: Several small cleanups in spans #116754

Merged
merged 14 commits into from
Oct 17, 2023
Merged

coverage: Several small cleanups in spans #116754

merged 14 commits into from
Oct 17, 2023

Conversation

Zalathar
Copy link
Contributor

While investigating the details of coverage span processing, I noticed several opportunities to make the code simpler and clearer.


@rustbot label +A-code-coverage

@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2023

r? @wesleywiser

(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. labels Oct 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2023

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 Oct 15, 2023
@Zalathar
Copy link
Contributor Author

Addressed review feedback (diff).

@Zalathar
Copy link
Contributor Author

Switched over to an if-let chain in the early-exit logic (diff).

@bors
Copy link
Contributor

bors commented Oct 15, 2023

☔ The latest upstream changes (presumably #116688) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_mir_transform/src/coverage/spans.rs Outdated Show resolved Hide resolved
debug!(" ...adding at least one pending={:?}", dup);
self.push_refined_span(dup);
}
// The list of dups is now empty, but we can recycle its capacity.
assert!(pending_dups.is_empty() && self.pending_dups.is_empty());
self.pending_dups = pending_dups;
} else {
self.pending_dups.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary anymore, right? Could assert that it is empty instead of clearing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this clear occurs, pending_dups should always be non-empty. We checked that it's non-empty at the top of the function, and in this branch we haven't tried to drain it.

(I wanted to prove this by adding the assert and having it fail, but this line doesn't seem to ever be executed by the current coverage test suite.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also makes me think that I should flatten the guard logic here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I wanted to prove this by adding the assert and having it fail, but this line doesn't seem to ever be executed by the current coverage test suite.)

could assert it and wait for someone to report an issue with coverage 😆

@Zalathar
Copy link
Contributor Author

Tweaked push_refined_span (diff).

@Zalathar
Copy link
Contributor Author

Flattened the guard logic in check_pending_dups/maybe_flush_pending_dups (diff).

This method's main responsibility is to flush the pending dups into refined
spans, if appropriate.
This patch also sorts the constructor fields into declaration order.
It turns out that all of the `len` manipulation here was just reimplementing
`last_mut`.
@Zalathar
Copy link
Contributor Author

Hoisted all of the renames to the start of the PR, so other changes can consistently refer to the new name instead of the old one.

This makes it easier to see that the non-initial cases assume that `prev` and
`curr` are set, and all operate on the same prev/curr references.
This reduces clutter, and makes it easier to notice regions where mutations
definitely don't occur.
Interacting with `basic_coverage_blocks` directly makes it easier to satisfy
the borrow checker when mutating `pending_dups` while reading other fields.
@Zalathar
Copy link
Contributor Author

Fixed indentation of a debug string (diff).

@oli-obk
Copy link
Contributor

oli-obk commented Oct 16, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Oct 16, 2023

📌 Commit 7aa1b83 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 Oct 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2023
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#116754 (coverage: Several small cleanups in `spans`)
 - rust-lang#116798 (Improve display of parallel jobs in rustdoc-gui tester script)
 - rust-lang#116800 (Fix implied outlives check for GAT in RPITIT)
 - rust-lang#116805 (Make `rustc_onunimplemented` export path agnostic)
 - rust-lang#116808 (Add myself to smir triage)
 - rust-lang#116811 (Preserve unicode escapes in format string literals when pretty-printing AST)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 581f88d into rust-lang:master Oct 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 17, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2023
Rollup merge of rust-lang#116754 - Zalathar:spans, r=oli-obk

coverage: Several small cleanups in `spans`

While investigating the details of coverage span processing, I noticed several opportunities to make the code simpler and clearer.

---

`@rustbot` label +A-code-coverage
@Zalathar Zalathar deleted the spans branch October 17, 2023 01:19
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
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.

6 participants