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

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thank you very much for doing this!

Logistics note: in the future, please upload a diff with more context (`git diff -U1000 ...`).


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:38
@@ -36,1 +37,3 @@
+#include <sys/types.h>
+#include <sys/stat.h>
 
----------------
I don't think you need any of these #includes.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:76
@@ -72,1 +75,3 @@
 
+  void walk(SmallString<128> Path);
+
----------------
Please add a short, doxygen-style comment describing what `walk` does. Also: `walk` does not seem like a sufficiently descriptive name. Wdyt of e.g `collectSourceFiles`?

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:76
@@ -72,1 +75,3 @@
 
+  void walk(SmallString<128> Path);
+
----------------
vsk wrote:
> Please add a short, doxygen-style comment describing what `walk` does. Also: `walk` does not seem like a sufficiently descriptive name. Wdyt of e.g `collectSourceFiles`?
I think a StringRef would work better than a SmallString here.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:248
@@ +247,3 @@
+void CodeCoverageTool::walk(SmallString<128> Path) {
+  struct stat info;
+  if (stat(std::string(Path.str()).c_str(), &info) != 0)
----------------
Please use `llvm::sys::fs::status`.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:249
@@ +248,3 @@
+  struct stat info;
+  if (stat(std::string(Path.str()).c_str(), &info) != 0)
+    errs() << "cannot access " << Path << "\n";
----------------
nit, I prefer checking for the simplest/base case first. Start by handling `llvm::sys::fs::is_regular_file(FS)`, followed by `llvm::sys::fs::is_directory(FS)`, followed by the error case.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:250
@@ +249,3 @@
+  if (stat(std::string(Path.str()).c_str(), &info) != 0)
+    errs() << "cannot access " << Path << "\n";
+  else if (info.st_mode & S_IFDIR) {
----------------
Please follow the existing error-reporting idiom in this file (e.g colored_ostream(), "error: Failed to ....").

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:257
@@ +256,3 @@
+         File != FileEnd && !EC; File.increment(EC)) {
+      if (stat(File->path().c_str(), &StatBuf))
+        continue;
----------------
Why not just call `walk` on `File->path()` directly? It should handle files, directories, and access errors correctly.


http://reviews.llvm.org/D20803