coverage: Replace boolean options with a CoverageLevel enum
#124507
Conversation
|
rustbot has assigned @compiler-errors. Use |
rustbot
added
S-waiting-on-review
|
It's nice to make this more cleaner. How about renaming the argument to |
|
For now, I would prefer to stick with I don't think having separate flags for “level” and ”options” is useful at the moment, and keeping everything under “options” reduces back-and-forth churn. In other words, I think we should keep all of these settings under |
| for option in v.split(',') { | ||
| match option { | ||
| "no-branch" => { | ||
| slot.branch = false; | ||
| slot.mcdc = false; | ||
| } | ||
| "branch" => slot.branch = true, | ||
| "mcdc" => { | ||
| slot.branch = true; | ||
| slot.mcdc = true; | ||
| } | ||
| "block" => slot.level = CoverageLevel::Block, | ||
| "branch" => slot.level = CoverageLevel::Branch, | ||
| "mcdc" => slot.level = CoverageLevel::Mcdc, | ||
| _ => return false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we keep the for loop for future changes ? Otherwise, I think we should get rid of it, as it wouldn't make sense to give -Z coverage-options=mcdc,block to the compiler
There was a problem hiding this comment.
I think we should keep the loop.
It's true that there are no other independent options at the moment, but I don't think there's much benefit in removing it now just to have to add it back later when adding another option.
There was a problem hiding this comment.
(Some examples of possible future options: #124144 (comment), #120013, and perhaps an option to not expand/emit zero-width spans.)
Zalathar
force-pushed
the
coverage-level
branch
from
e8cad91
to
f926337
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (f9dca46): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.628s -> 673.064s (-0.23%) |
After #123409, and some discussion at #79649 (comment) and #124120, it became clear to me that we should have a unified concept of “coverage level”, instead of having several separate boolean flags that aren't actually independent.
This PR therefore introduces a
CoverageLevelenum, to replace the existing boolean flags forbranchandmcdc.The
no-branchvalue (for-Zcoverage-options) has been renamed toblock, instructing the compiler to only instrument for block coverage, with no branch coverage or MD/DC instrumentation.@rustbot label +A-code-coverage
cc @ZhuUx @Lambdaris @RenjiSann