Page MenuHomePhabricator

[LLVM-COV] Add the coverage of lines in the summary report.
ClosedPublic

Authored by MaggieYi on Jul 20 2016, 7:09 AM.

Details

Summary

The llvm-cov ‘report' command displays a summary of the coverage of a binary file.
The summary report currently only includes covered regions and covered functions as shown below:

Filename Regions Miss Cover Functions Executed
I:\test\test.cpp 12 2 83.33% 3 100.00%
TOTAL 12 2 83.33% 3 100.00%

This patch adds the coverage of lines in the summary report, for example:

Filename Regions Miss Cover Functions Miss Executed Lines Miss Cover
I:\test\test.cpp 12 2 83.33% 3 0 100.00% 26 2 92.31%
TOTAL 12 2 83.33% 3 0 100.00% 26 2 92.31%

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 64682.Jul 20 2016, 7:09 AM
MaggieYi retitled this revision from to [LLVM-COV] Add the coverage of lines in the summary report..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, harlanhaskins, bogner.
MaggieYi added a subscriber: llvm-commits.
MaggieYi updated this object.Jul 20 2016, 7:12 AM
vsk edited edge metadata.Jul 20 2016, 11:07 AM

Nice! This looks really close.

Apart from the nits I mentioned, please update the test in test/tools/llvm-cov/prevent_false_instantiations.h. It currently checks for:

NAN-NOT: 0{{[ \t]+}}nan%{{[ \t]+}}0{{[ \t]+}}nan%

But with the extra columns you added, it should be relaxed to:

NAN-NOT: {{[ \t]+}}nan%

test/tools/llvm-cov/report.cpp
4 ↗(On Diff #64682)

Could you rename the three "Miss" columns to {"Missed Regions", "Missed Functions", "Missed Lines"}? I think that would make the report less confusing.

tools/llvm-cov/CoverageReport.cpp
91 ↗(On Diff #64682)

The comment here doesn't exactly match the source. I'd prefer something more general, e.g: "Specify the default column widths."

MaggieYi updated this revision to Diff 64881.Jul 21 2016, 7:13 AM
MaggieYi edited edge metadata.

Thanks Vedant. I have updated the patch following your comments.

MaggieYi marked 2 inline comments as done.Jul 21 2016, 7:17 AM

The test in test/tools/llvm-cov/prevent_false_instantiations.h has been updated.

test/tools/llvm-cov/report.cpp
4 ↗(On Diff #64881)

I have renamed the three ‘Miss’ heading to follow your advice. I have changed the default column widths to:

static size_t FileReportColumns[] = {25, 12, 18, 10, 12, 18, 10, 12, 18, 10};

These new ‘Miss’ columns widths are larger than the other columns. I think the report looks less nice, what do you think?

tools/llvm-cov/CoverageReport.cpp
91–92 ↗(On Diff #64881)

Yes, I have done it.

vsk accepted this revision.Jul 21 2016, 10:06 AM
vsk edited edge metadata.

LGTM.

test/tools/llvm-cov/report.cpp
4 ↗(On Diff #64881)

IMO the report would look nicer with separators between the column names (e.g: "Filename | Regions | ... | Cover") . We can do that in a follow-up patch. I don't think re-using column names is the right answer.

This revision is now accepted and ready to land.Jul 21 2016, 10:06 AM
This revision was automatically updated to reflect the committed changes.