[PATCH] D23087: [LLVM-COV] Replace tabs to 2-space indentation in the HTML coverage report.

[Vedant Kumar via llvm-commits]

vsk added a comment.

Mostly looks good. The logic in escape() looks correct.


================
Comment at: test/tools/llvm-cov/showTabsHTML.cpp:3
@@ +2,3 @@
+// RUN: llvm-cov show %S/Inputs/showTabsHTML.covmapping -format html -instr-profile %t.profdata -filename-equivalence %s | FileCheck -check-prefix=CHECK %s
+// RUN: llvm-cov show %S/Inputs/showTabsHTML.covmapping -format html -instr-profile %t.profdata -filename-equivalence -name=main %s | FileCheck -check-prefix=CHECK  %s
+
----------------
Does the second RUN line achieve any additional test coverage for this patch? If not, please remove it.

================
Comment at: test/tools/llvm-cov/showTabsHTML.cpp:21
@@ +20,3 @@
+  return 0;
+}
+
----------------
Since this test is just checking whether tabs are expanded properly, please keep the CHECKs on lines {9, 14, 18} and remove the rest. That should make the test easier to maintain.

================
Comment at: test/tools/llvm-cov/showTabsHTML.cpp:38
@@ +37,1 @@
+// CHECK-TABSIZE: (void) "This     tab starts at column 15";
\ No newline at end of file

----------------
Ditto, please trim this set of CHECKs.

================
Comment at: tools/llvm-cov/CoverageViewOptions.h:37
@@ -36,2 +36,3 @@
   std::vector<std::string> DemanglerOpts;
+  static uint32_t TabSize;
 
----------------
Please don't introduce static members -- it should be possible to pass a CoverageViewOptions instance instead.

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:46
@@ +45,3 @@
+                           (--ColNum % CoverageViewOptions::TabSize);
+      for (unsigned i = 0; i < NumSpaces; ++i)
+        Result += " ";
----------------
Capitalize the iterator variable name.


https://reviews.llvm.org/D23087