[PATCH] D23345: [llvm-cov] Add the project summary to each source file coverage report.

[Ying Yi via llvm-commits]

MaggieYi added inline comments.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:470
@@ -466,1 +469,3 @@
+    sys::path::native(ObjectFilePath);
+    ViewOpts.ObjectFilename = ObjectFilePath.c_str();
     switch (ViewOpts.Format) {
----------------
vsk wrote:
> We aren't calling sys::path::native in CoveragePrinter::getOutputPath() either. We should make the change to ObjectFilename and getOutputPath in the same patch, and make sure to test it with a covmapping file that includes Windows path separators. Could you tackle this in a separate patch?
Ok, I will create a separate patch for this issue.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:633
@@ +632,3 @@
+  SourceCoverageView::CreatedTimeStr =
+      "Created: " + ModifiedTimeStr.substr(0, found).str();
+
----------------
vsk wrote:
> It's not clear that the results of the find+substr would be uniform across platforms. I suggest just using the result of `MTime.str()`.
I checked the generated report on Linux and Windows. The created time formats are the same, which is “Created: 16-01-07 13:12”. If using “MTime.st()”, the created times will be “Created: 16-01-07 13:12:48.216418700” on both Linux and Windows. Since the time string is quite long, I change the code to output the full time string if it can’t find the “:”. What do you think?

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:340
@@ +339,3 @@
+  SmallString<128> ObjectFilePath(getOptions().ObjectFilename);
+  SmallString<128> ObjectFile("Binary file:        ");
+  ObjectFile.append(ObjectFilePath);
----------------
vsk wrote:
> Nit, mild preference for "Binary:" over "Binary file:". Also, why are there extra spaces here?
Removed the extra spaces. 

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:350
@@ +349,3 @@
+  SourceFile = isFunctionView() ? "Function: " + SourceFile
+                                : "Source file:        " + SourceFile;
+  OS << tag("pre", escape(SourceFile, getOptions()));
----------------
vsk wrote:
> Why are there extra spaces after "Source file:"?
> 
> Nit, I'm mildly in favor of shortening "Source file" to "File", but I'll leave it up to you.
Again, removed the extra space. How about using “Source”?


https://reviews.llvm.org/D23345