[PATCH] D23486: [llvm-cov] Add coverage summary information to the HTML index page.
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Sat Aug 13 12:28:38 PDT 2016
vsk added a comment.
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.
================
Comment at: SourceCoverageViewHTML.cpp:20
@@ -18,2 +19,3 @@
#include "llvm/Support/Path.h"
+#include "llvm/Support/Format.h"
----------------
Nitpick: includes should be sorted.
================
Comment at: SourceCoverageViewHTML.cpp:213
@@ -211,1 +212,3 @@
+ const std::string &PathToStyle = "",
+ bool trAlternate = false) {
OS << "<!doctype html>"
----------------
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?
================
Comment at: SourceCoverageViewHTML.cpp:276
@@ +275,3 @@
+
+template<typename T> void sendFormat(raw_ostream &OSRef, format_object<T> value,
+ const char *align,
----------------
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.
================
Comment at: SourceCoverageViewHTML.cpp:289
@@ +288,3 @@
+
+static void showSummary(raw_ostream &OSRef, std::string name,
+ FileCoverageSummary& File)
----------------
Parameter names should be capitalized.
================
Comment at: SourceCoverageViewHTML.cpp:295
@@ +294,3 @@
+
+ sendFormat(OSRef, format("%u", (unsigned)File.RegionCoverage.NumRegions),
+ "right");
----------------
IIRC you can just use "%zu" for these and drop the casts. If not, it's OK as-is.
================
Comment at: SourceCoverageViewHTML.cpp:314
@@ +313,3 @@
+ "right", determineCoveragePercentageColor(File.LineCoverage));
+#if RICH
+ Options.colored_ostream(
----------------
Delete.
================
Comment at: SourceCoverageViewText.cpp:31
@@ -31,1 +30,3 @@
+Error CoveragePrinterText::createIndexFile(ArrayRef<StringRef> SourceFiles,
+ const coverage::CoverageMapping &Coverage) {
auto OSOrErr = createOutputStream("index", "txt", /*InToplevel=*/true);
----------------
Since it isn't used in this implementation, `const coverage::CoverageMapping &Coverage` -> `const coverage::CoverageMapping &`.
https://reviews.llvm.org/D23486
More information about the llvm-commits
mailing list