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

[Vedant Kumar via llvm-commits]

vsk added a comment.

This is looking really close. Just a few minor comments --


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:631
@@ +630,3 @@
+  auto ModifiedTime = Status.getLastModificationTime();
+  StringRef ModifiedTimeStr = ModifiedTime.str();
+  size_t found = ModifiedTimeStr.rfind(":");
----------------
This takes a reference to a temporary, which might make asan upset. Please use std::string.

================
Comment at: tools/llvm-cov/CodeCoverage.cpp:634
@@ +633,3 @@
+  ViewOpts.CreatedTimeStr =
+      (found != std::string::npos)
+          ? "Created: " + ModifiedTimeStr.substr(0, found).str()
----------------
Sgtm, thanks.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:242
@@ -238,1 +241,3 @@
 
+  /// \brief Render the report title and the created time for the view.
+  virtual void renderCellInTitle(raw_ostream &, StringRef CellText) = 0;
----------------
Please update the doxygen here to mention that the function also prints \p CellText.

================
Comment at: tools/llvm-cov/SourceCoverageView.h:243
@@ +242,3 @@
+  /// \brief Render the report title and the created time for the view.
+  virtual void renderCellInTitle(raw_ostream &, StringRef CellText) = 0;
+
----------------
Missing parameter name.

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:343
@@ +342,3 @@
+  SourceFile = isFunctionView() ? "Function: " + SourceFile
+                                : "Source: " + SourceFile;
+  OS << tag("pre", escape(SourceFile, getOptions()));
----------------
Nit, this copies the source name twice. How about 'SourceFile = "Function" or "Source"; SourceFile += getSourceName();'?

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:347
@@ +346,3 @@
+  if (WholeFile) {
+    std::string ObjectFile("Binary: ");
+    ObjectFile.append(getOptions().ObjectFilename);
----------------
Sgtm.

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:349
@@ +348,3 @@
+    ObjectFile.append(getOptions().ObjectFilename);
+    OS << tag("pre", escape(ObjectFile, getOptions()));
+  }
----------------
Nit, how about 'escape("Binary: " + getOptions().ObjectFilename)'?

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:552
@@ +551,3 @@
+     << tag("span", escape(getOptions().ProjectTitle, getOptions()))
+     << EndProjectTitleDiv;
+
----------------
Does this need to be gated with a check to hasProjectTitle()?

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:559
@@ +558,3 @@
+     << tag("span", escape(getOptions().CreatedTimeStr, getOptions()))
+     << EndCreatedTimeDiv;
+}
----------------
Ditto, do we need a check here for hasCreatedTime()?

================
Comment at: tools/llvm-cov/SourceCoverageViewHTML.h:73
@@ -72,1 +72,3 @@
 
+  void renderCellInTitle(raw_ostream &, StringRef CellText) override;
+
----------------
Missing parameter name.

================
Comment at: tools/llvm-cov/SourceCoverageViewText.cpp:225
@@ +224,3 @@
+  getOptions().colored_ostream(OS, raw_ostream::CYAN)
+      << getOptions().CreatedTimeStr << "\n";
+}
----------------
Ditto, do we need checks here for hasProjectTitle() / hasCreatedTime()?


https://reviews.llvm.org/D23345