[PATCH] D20803: Displaying coverage information for all source files present in a directory

[Vedant Kumar via llvm-commits]

vsk added inline comments.

================
Comment at: test/tools/llvm-cov/cov_dir.cpp:5
@@ +4,3 @@
+//RUN: echo "" > %t/a/d.tmp
+//RUN: llvm-cov show %S/Inputs/directory-search.covmapping -instr-profile=/dev/null -dump-input-file-list %t | FileCheck %s
+//CHECK-DAG: {{.*}}c.tmp
----------------
It might make more sense to pass "/dev/null" for the covmapping file, since it ultimately isn't used. Also, please add another RUN line to test your code with `-filename-equivalence` turned on.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:96
@@ -92,2 +95,3 @@
   std::string CoverageArch;
+  bool  DumpFileNames;
 
----------------
Does this really need to live here? Why not just deal with it in parseCommandLineArgs?

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:257
@@ +256,3 @@
+void CodeCoverageTool::collectSourceFiles(StringRef Path) {
+  if (CompareFilenamesOnly) {
+    addCollectedPath(Path.str());
----------------
Why are we skipping the recursive directory traversal if -filename-equivalence is turned on?

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:279
@@ +278,3 @@
+      }
+      return;
+    }
----------------
Drop the redundant return statement here.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:282
@@ +281,3 @@
+  } else
+    fprintf(stderr, "Error: Can't access: %s\n", Path.str().c_str());
+  return;
----------------
Use the `error` method instead of fprintf.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:358
@@ -323,1 +357,3 @@
 
+  cl::opt<bool> DumpInputFileList(
+      "dump-input-file-list", cl::Optional,
----------------
You're only handling this argument when running the "show" sub-command. It would be better to handle it in commandLineParser.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:475
@@ +474,3 @@
+  if (DumpFileNames) {
+    for (auto it = SourceFiles.begin(); it != SourceFiles.end(); it++)
+      llvm::outs() << *it << "\n";
----------------
In llvm, this is typically written using a range-based for, e.g: `for (StringRef SF : SourceFiles)`.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:476
@@ +475,3 @@
+    for (auto it = SourceFiles.begin(); it != SourceFiles.end(); it++)
+      llvm::outs() << *it << "\n";
+    return 0;
----------------
I think this file uses the llvm namespace, so you should be able to drop the `llvm::`.


http://reviews.llvm.org/D20803