[PATCH] D23486: [llvm-cov] Add coverage summary information to the HTML index page.

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



