Make location coverage reported include all lines of a multi-line statement #5635
Conversation
As there is no other goto program in scope the underscore is not needed for avoiding naming clashes. Therefore this rename can be done in order to help with readability.
So that additional code can be added to this part in the following commit without increasing the size of the constructor.
Because the entire statement is part of the block.
thomasspriggs
requested review from
chrisr-diffblue,
hannes-steffenhagen-diffblue,
martin-cs,
peterschrammel and
smowton
as code owners
thomasspriggs
force-pushed
the
tas/expression_coverage
branch
from
8f667c9 to
4af02ce
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5635 +/- ##
========================================
Coverage 69.36% 69.36%
========================================
Files 1241 1241
Lines 100564 100571 +7
========================================
+ Hits 69753 69760 +7
Misses 30811 30811
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
To show that the new updated printing is working correctly.
thomasspriggs
force-pushed
the
tas/expression_coverage
branch
from
4af02ce to
b156c8f
Compare
| @@ -29,12 +29,12 @@ optionalt<std::size_t> cover_basic_blockst::continuation_of_block( | |||
| return {}; | |||
| } | |||
|
|
|||
| cover_basic_blockst::cover_basic_blockst(const goto_programt &_goto_program) | |||
| cover_basic_blockst::cover_basic_blockst(const goto_programt &goto_program) | |||
| block_info.lines.insert(unsafe_string2unsigned(id2string(line))); | ||
| block_info.source_lines.insert(it->source_location); | ||
| } | ||
| add_block_lines(block_info, *it); |
There was a problem hiding this comment.
This is really unfortunate. Constructors generally shouldn’t be as complicated as this one is to begin with!
(Not that this is this PRs fault)
| @@ -155,6 +149,18 @@ void cover_basic_blockst::output(std::ostream &out) const | |||
| << '\n'; | |||
| } | |||
|
|
|||
| void cover_basic_blockst::add_block_lines( | |||
| cover_basic_blockst::block_infot &block, | |||
There was a problem hiding this comment.
⛏️ should imho be called block_info.
There was a problem hiding this comment.
IMHO, it would be better to rename block_infot to blockt. This is on the basis that you wouldn't bother creating instances of a struct that didn't contain information. Therefore the word info is redundant in the context of the name. It doesn't really convey any useful meaning to the reader.
| const irep_idt &line = location.get_line(); | ||
| if(!line.empty()) | ||
| { | ||
| block.lines.insert(unsafe_string2unsigned(id2string(line))); |
There was a problem hiding this comment.
❓ block_infot::lines is a set I presume?
| @@ -0,0 +1,2 @@ | |||
|
|
|||
There was a problem hiding this comment.
⛏️ Could use a comment explaining what this is here
There was a problem hiding this comment.
It is already explained in a comment of the .desc file.
This PR makes location coverage reported include all lines of multi-line statements. Previously this would only report the line on which a multi-line statement begins. This could falsely give the impression that the end of a statement was not covered.
This should resolve part of the inconstancy seen in #5543
My commit message includes data points confirming performance improvements (if claimed).