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/
Details
Diff Detail
Event Timeline
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 &. |
Ran clang-format on the files. Made most requested changes. CSS style change still needed.
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? |
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? |
CodeCoverage.cpp | ||
---|---|---|
655 | You don’t need to pass the ‘SourceFiles’ as a parameter anymore since you can get them by calling Coverage->getUniqueSourceFiles(). |
The final source changes (I think!). Still need to re-learn lit and do the test case.
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. |
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.
You don’t need to pass the ‘SourceFiles’ as a parameter anymore since you can get them by calling Coverage->getUniqueSourceFiles().