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

[Vedant Kumar via llvm-commits]

vsk added inline comments.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:467
@@ +466,3 @@
+    SmallString<128> ObjectFilePath(this->ObjectFilename);
+    sys::fs::make_absolute(ObjectFilePath);
+    // Canonicalize object file path to a native path to avoid mixed separator.
----------------
We should handle the error_code make_absolute returns (CodeCoverageTool::warning).

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:470
@@ -466,1 +469,3 @@
+    sys::path::native(ObjectFilePath);
+    ViewOpts.ObjectFilename = ObjectFilePath.c_str();
     switch (ViewOpts.Format) {
----------------
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?

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:627
@@ +626,3 @@
+        << "profdata file error: can not get the file status. \n";
+    return 1;
+  }
----------------
Use CodeCoverageTool::error.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:632
@@ +631,3 @@
+  size_t found = ModifiedTimeStr.rfind(":");
+  SourceCoverageView::CreatedTimeStr =
+      "Created: " + ModifiedTimeStr.substr(0, found).str();
----------------
Please sink the global into CoverageViewOptions, so there's just one place we need to look for information like this.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:633
@@ +632,3 @@
+  SourceCoverageView::CreatedTimeStr =
+      "Created: " + ModifiedTimeStr.substr(0, found).str();
+
----------------
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()`.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:175
@@ -174,1 +174,3 @@
 
+  /// Return true if this view is a function view, otherwise return false by default.
+  bool FunctionView;
----------------
Nit about the comment: since this is a field, the word "return" seems out of place. Wdyt of "Specifies whether or not ..."?

================
Comment at: tools/llvm-cov/SourceCoverageView.h:201
@@ -196,1 +200,3 @@
+  /// \brief Render the object file name for the view.
+  virtual void renderObjectFilename(raw_ostream &OS) = 0;
 
----------------
This method does not need to be pure-virtual. Please delete it and move its logic into the renderSourceName implementations.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:252
@@ +251,3 @@
+  /// \brief Render the created time for the view.
+  virtual void renderCreatedTime(raw_ostream &OS) = 0;
+
----------------
I think we should delete all three of these methods and replace them with something simpler: `virtual void renderCellInTitle(raw_ostream &, StringRef CellText)`. The text viewer would provide an implementation that prints "CellText" in cyan followed by a newline. The html viewer would do something analogous.

This would force some common logic to move into SourceCoverageView::print, which is a good thing. E.g, we'd no longer have inconsistent capitalization between "renderReportTitle" in the text vs. html viewers.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:255
@@ +254,3 @@
+  /// \brief Render the project summary for a given source file name.
+  void renderProjectSummary(raw_ostream &OS);
+
----------------
This isn't pure-virtual, so it doesn't belong in this section of SourceCoverageView. Please delete it and move its logic to SourceCoverageView::print.

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:339
@@ +338,3 @@
+void SourceCoverageViewHTML::renderObjectFilename(raw_ostream &OS) {
+  SmallString<128> ObjectFilePath(getOptions().ObjectFilename);
+  SmallString<128> ObjectFile("Binary file:        ");
----------------
What's wrong with using normal std::string concatenation?

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

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:350
@@ +349,3 @@
+  SourceFile = isFunctionView() ? "Function: " + SourceFile
+                                : "Source file:        " + SourceFile;
+  OS << tag("pre", escape(SourceFile, getOptions()));
----------------
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.

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:571
@@ +570,3 @@
+void SourceCoverageViewHTML::renderTableHeader(raw_ostream &OS,
+                                             unsigned ViewDepth) {
+  renderLinePrefix(OS, ViewDepth);
----------------
clang-format

================
Comment at: tools/llvm-cov/SourceCoverageViewText.cpp:241
@@ +240,3 @@
+  getOptions().colored_ostream(OS, raw_ostream::WHITE)
+      << " Line|  Count|Source\n";
+  renderLineSuffix(OS, ViewDepth);
----------------
This looks really suspicious. I don't think this will look right with large line numbers or large execution counts. I suggest just defining this away for the text view, e.g: `SourceCoverageViewText::renderTableHeader(raw_ostream &, unsigned)`.


https://reviews.llvm.org/D23345