Page MenuHomePhabricator

[llvm-cov] Add coverage summary information to the HTML index page.
Needs ReviewPublic

Authored by rdp on Aug 13 2016, 8:05 AM.

Details

Reviewers
vsk
Summary

Added the information output by "llvm-cov report" to the "llvm-cov show -format html" index.html page. An example of the output can be seen at http://ellcc.org/coverage/

Diff Detail

Event Timeline

rdp updated this revision to Diff 67962.Aug 13 2016, 8:05 AM
rdp retitled this revision from to [llvm-cov] Add coverage summary information to the HTML index page..
rdp updated this object.
rdp added a reviewer: vsk.
rdp added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 13 2016, 12:28 PM

Thanks for the patch! I think this is the right way forward. I'd like to get to the point where the CoverageReport class exposes this functionality for use by createIndexFile, but that's a simple refactoring we can do later.

On a high-level, this patch is missing a lit-style test. Please write a test that generates a coverage index in html format and verifies that its counts are correct. The best way to do this is to copy llvm/test/tools/llvm-cov/style.test, pare it down (remove extraneous RUN/CHECK lines), and add checks to verify the contents of the index.

Also, this diff needs to be run through clang-format to match the llvm style.

My other comments are pretty minor.

SourceCoverageViewHTML.cpp
21

Nitpick: includes should be sorted.

212

We should move the logic for selecting the right CSS away from the logic to print out the header. Can you define something like: std::string getCSSForCoverage(PathToStyle = "", AlternateRowColor = false), and have the callers of emitPrelude pass in the right CSS?

287

I suggest renaming this function to better reflect what it's doing (maybe emitFormattedCount?). Also, parameter names should be capitalized, nullptr is preferred over NULL, and Optional<StringRef> is preferred over default null parameters.

300

Parameter names should be capitalized.

306

IIRC you can just use "%zu" for these and drop the casts. If not, it's OK as-is.

325

Delete.

SourceCoverageViewText.cpp
31

Since it isn't used in this implementation, const coverage::CoverageMapping &Coverage -> const coverage::CoverageMapping &.

rdp updated this revision to Diff 67968.Aug 13 2016, 8:45 PM
rdp edited edge metadata.

Ran clang-format on the files. Made most requested changes. CSS style change still needed.

rdp marked 5 inline comments as done.Aug 13 2016, 8:52 PM

This is my first time using Phabricator. I hope I updated the review properly.

SourceCoverageViewHTML.cpp
212

I think I understand, but I'm not an HTML/CSS expert. I think I want the CSSForCoverage used for every page, and the AlternateRowColor added for just the index page. Are you suggesting that getCSSForCoverage return the string for the entire CSS?

vsk added a comment.Aug 14 2016, 11:51 AM

This is the right way to update a review :).

SourceCoverageViewHTML.cpp
212

We're on the same page: with this change, CSSForCoverage will still be used in each file. I'm suggesting that getCSSForCoverage return either a link to a stylesheet or a full CSS blob. In either case, it should be able to optionally add the CSS you wrote to alternate row colors.

288

I'm curious: does it work to drop the template stuff and pass in a const format_object_base & instead of format_object<T>?

299

Can you make Name a const std::string & or a StringRef?

MaggieYi added inline comments.
CodeCoverage.cpp
655

You don’t need to pass the ‘SourceFiles’ as a parameter anymore since you can get them by calling Coverage->getUniqueSourceFiles().

vsk added a comment.Aug 16 2016, 10:19 AM

@rdp I noticed that you held off on adding a test with your last update. Let me know if you'd like any help with that -- I can take a shot at it if needed.

CodeCoverage.cpp
655

Actually, I think we still need it here to ensure addCollectedPath() + -filename-equivalence continues working as expected.

rdp updated this revision to Diff 68264.Aug 16 2016, 2:57 PM

The final source changes (I think!). Still need to re-learn lit and do the test case.

vsk added inline comments.Aug 16 2016, 3:10 PM
SourceCoverageView.h
145–146

The set of SourceFiles being passed to createIndexFile may not be the same as the Coverage.getUniqueSourceFiles(). Please keep the SourceFiles argument.

SourceCoverageViewHTML.cpp
268

Looks good.

SourceCoverageViewText.cpp
94

Ditto -- formatting-only changes belong in a separate commit.

168

Since this change doesn't affect your patch's functionality, let's do it in a follow-up commit.

rdp updated this revision to Diff 68361.Aug 17 2016, 8:30 AM
rdp marked an inline comment as done.

Reverted SourceFiles changes. Removed unrelated formatting.

rdp marked 3 inline comments as done.Sep 5 2016, 8:06 AM
vsk added a comment.Sep 9 2016, 11:22 AM

Hi @rdp, I went ahead and threaded information from CoverageReport into the text and html indices (see: r281062). Please take a look and let me know if there's anything you'd like changed.