[llvm-cov] Provided easy navigation to find the previous and next report.

[MaggieYi]

Summary

This patch provided easy navigation to find the previous and next source file report. For each source file view, three hyperlinks (index, prev and next links) are given below the report create time. By clicking the index link, the coverage report will go back to index.html page. The prev/next links will go to the previous/next source report. The prev link is disabled if the source file is the first source file link in the index page. The next link is disabled if the source file is the last source file link in the index page.

[vsk]

On a low level, I'm concerned about the unnecessary quadratic behavior in this patch. If "N = |SourceFiles|", then renderPageNavigation() does O(N) work and it's called O(N) times. Given that llvm+clang has over 2,400 source files, this could actually be noticeable. We already get user reports about llvm-cov being slow...

On a high level, I'm not convinced that this is a significant usability improvement (as opposed to the "Go to first unexecuted line" feature). You can emulate this functionality by clicking the "Back" button in your browser and picking the next interesting file. Note that users are generally uninterested in looking at the coverage report for *every* file -- they usually just care about the ones with low coverage.

I have some alternate ideas for how to improve the html reports if you're interested: 1) include a list of unexecuted functions in a file with links to the right source locations, 2) implement a function whitelist (e.g, the "dump()" function in llvm is never executed, but shouldn't be marked as uncovered).

test/tools/llvm-cov/showProjectSummary.cpp

35

I don't think we should display "Prev" or "Next" if they aren't links.

35

We're missing tests for the case where there is a prev link but no next link, and a next link but no prev link. Maybe you could reuse the multiple-inputs.covmapping file to test this?

[MaggieYi]

In D24367#539047, @vsk wrote:

On a low level, I'm concerned about the unnecessary quadratic behavior in this patch. If "N = |SourceFiles|", then renderPageNavigation() does O(N) work and it's called O(N) times. Given that llvm+clang has over 2,400 source files, this could actually be noticeable. We already get user reports about llvm-cov being slow...

Thanks for pointing this out. I have updated the patch and removed unnecessary execution time in the renderPageNavigation() function. After reducing the cost of running time, will it be considered as an acceptable feature?

On a high level, I'm not convinced that this is a significant usability improvement (as opposed to the "Go to first unexecuted line" feature). You can emulate this functionality by clicking the "Back" button in your browser and picking the next interesting file. Note that users are generally uninterested in looking at the coverage report for *every* file -- they usually just care about the ones with low coverage.

I will add filter and sort table functions to the index page to only show the source files that the users are interested in. In the long term, the previous and next links could then automatically update when using the filter function.

[MaggieYi]

test/tools/llvm-cov/showProjectSummary.cpp

35

Thanks, I have followed your advice to add the missing tests.

[vsk]

In D24367#541163, @MaggieYi wrote:

In D24367#539047, @vsk wrote:

On a low level, I'm concerned about the unnecessary quadratic behavior in this patch. If "N = |SourceFiles|", then renderPageNavigation() does O(N) work and it's called O(N) times. Given that llvm+clang has over 2,400 source files, this could actually be noticeable. We already get user reports about llvm-cov being slow...

Thanks for pointing this out. I have updated the patch and removed unnecessary execution time in the renderPageNavigation() function. After reducing the cost of running time, will it be considered as an acceptable feature?

I didn't mean to request changes to this diff with my inline comments. Sorry for the confusion. Let's figure out whether the feature is needed and worry about the code later.

On a high level, I'm not convinced that this is a significant usability improvement (as opposed to the "Go to first unexecuted line" feature). You can emulate this functionality by clicking the "Back" button in your browser and picking the next interesting file. Note that users are generally uninterested in looking at the coverage report for *every* file -- they usually just care about the ones with low coverage.

I will add filter and sort table functions to the index page to only show the source files that the users are interested in. In the long term, the previous and next links could then automatically update when using the filter function.

Being able to sort the entries of the index file by, e.g function coverage percentage would be nice. However, I don't think the Next/Prev links would be compelling even in this context. It just makes the difference between two clicks (back button + next file report link) versus one, for a relatively infrequent action. I don't think we should go ahead with this patch.

[MaggieYi]

I agree with you and abandon this patch.

test/tools/llvm-cov/f1.c

+int f1() {}
+
+// RUN: llvm-profdata merge %S/Inputs/multiple-files.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/multiple-files.covmapping -format=html -o %t.dir -instr-profile %t.profdata -filename-equivalence %S/f1.c %S/f2.c %S/f3.c
+// RUN: FileCheck -check-prefixes=HTML -input-file %t.dir/coverage/tmp/coverage/f1.c.html %s
+// HTML: <a href='../../../index.html'>Index</a>
+// HTML-NOT: >Prev<
+// HTML: <a href='../../../coverage/tmp/coverage/a/f2.c.html'>Next</a>

test/tools/llvm-cov/f2.c

+int f2() {}
+
+// RUN: llvm-profdata merge %S/Inputs/multiple-files.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/multiple-files.covmapping -format=html -o %t.dir -instr-profile %t.profdata -filename-equivalence %S/f1.c %S/f2.c %S/f3.c
+// RUN: FileCheck -check-prefixes=HTML -input-file %t.dir/coverage/tmp/coverage/a/f2.c.html %s
+// HTML: <a href='../../../../index.html'>Index</a>
+// HTML: <a href='../../../../coverage/tmp/coverage/f1.c.html'>Prev</a>
+// HTML: <a href='../../../../coverage/tmp/coverage/b/f3.c.html'>Next</a>

test/tools/llvm-cov/f3.c

+int f3() {}
+
+// RUN: llvm-profdata merge %S/Inputs/multiple-files.proftext -o %t.profdata
+// RUN: llvm-cov show %S/Inputs/multiple-files.covmapping -format=html -o %t.dir -instr-profile %t.profdata -filename-equivalence %S/f1.c %S/f2.c %S/f3.c
+// RUN: FileCheck -check-prefixes=HTML -input-file %t.dir/coverage/tmp/coverage/b/f3.c.html %s
+// HTML: <a href='../../../../index.html'>Index</a>
+// HTML: <a href='../../../../coverage/tmp/coverage/a/f2.c.html'>Prev</a>
+// HTML-NOT: >Next<

test/tools/llvm-cov/showProjectSummary.cpp

 // TEXT: Created:
 // TEXT-FILE: showProjectSummary.cpp:
 // TEXT-FILE: showProjectSummary.covmapping:
 // TEXT-FUNCTION: main:
 // TEXT-FOOTER: Generated by llvm-cov
 
 // Test html output.
 // RUN: llvm-cov show %S/Inputs/showProjectSummary.covmapping -format=html -o %t.dir -instr-profile %t.profdata -filename-equivalence %s
-// RUN: FileCheck -check-prefixes=HTML,HTML-FILE,HTML-HEADER,HTML-FOOTER -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
+// RUN: FileCheck -check-prefixes=HTML,HTML-FILE,HTML-HEADER,HTML-FOOTER,HTML-NAVIGATION -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
 // RUN: llvm-cov show %S/Inputs/showProjectSummary.covmapping -format=html -o %t.dir -instr-profile %t.profdata -project-title "Test Suite" -filename-equivalence %s
 // RUN: FileCheck -check-prefixes=HTML-TITLE,HTML,HTML-FILE,HTML-HEADER,HTML-FOOTER -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
 // RUN: FileCheck -check-prefixes=HTML-TITLE,HTML,HTML-FOOTER -input-file %t.dir/index.html %s
 // RUN: llvm-cov show %S/Inputs/showProjectSummary.covmapping -format=html -o %t.dir -instr-profile %t.profdata  -project-title "Test Suite" -filename-equivalence -name=main %s
 // RUN: FileCheck -check-prefixes=HTML-FUNCTION,HTML-HEADER,HTML-FOOTER -input-file %t.dir/functions.html %s
 // HTML-TITLE: <h1>Test Suite</h1>
 // HTML: <h2>Coverage Report</h2>
 // HTML: <h4>Created:{{.*}}</h4>
+// HTML-NAVIGATION: <a href='../../index.html'>Index</a>
 // HTML-FILE: <pre>{{.*}}showProjectSummary.cpp (Binary: showProjectSummary.covmapping)</pre>
 // HTML-FUNCTION: <pre>main</pre>
 // HTML-HEADER: <td><pre>Line No.</pre></td>
 // HTML-HEADER: <td><pre>Count</pre></td>
 // HTML-HEADER: <td><pre>Source (<a href='#L8'>jump to first uncovered line</a>)</pre></td>
 // HTML-FOOTER: <h5>Generated by llvm-cov{{.*}}</h5>

tools/llvm-cov/CodeCoverage.cpp

int CodeCoverageTool::show(int argc, const char **argv,
   }
 
   // In -output-dir mode, it's safe to use multiple threads to print files.
   unsigned ThreadCount = 1;
   if (ViewOpts.hasOutputDirectory())
     ThreadCount = std::thread::hardware_concurrency();
   ThreadPool Pool(ThreadCount);
 
-  for (StringRef SourceFile : SourceFiles) {
-    Pool.async([this, SourceFile, &Coverage, &Printer, ShowFilenames] {
+  for (size_t I = 0, E = SourceFiles.size(); I < E; ++I) {
+    StringRef SourceFile = SourceFiles[I];
+    Pool.async([this, SourceFile, &Coverage, &Printer, ShowFilenames, I] {
       auto View = createSourceFileView(SourceFile, *Coverage);
       if (!View) {
         warning("The file '" + SourceFile.str() + "' isn't covered.");
         return;
       }
 
       auto OSOrErr = Printer->createViewFile(SourceFile, /*InToplevel=*/false);
       if (Error E = OSOrErr.takeError()) {
         error("Could not create view file!", toString(std::move(E)));
         return;
       }
       auto OS = std::move(OSOrErr.get());
 
       View->print(*OS.get(), /*Wholefile=*/true,
-                  /*ShowSourceName=*/ShowFilenames);
+                  /*ShowSourceName=*/ShowFilenames, /*ViewDepth=*/0,
+                  SourceFiles, I);
       Printer->closeViewFile(std::move(OS));
     });
   }
 
   Pool.wait();
 
   return 0;
 }

tools/llvm-cov/CoverageViewOptions.h

struct CoverageViewOptions {
   bool hasCreatedTime() const { return !CreatedTimeStr.empty(); }
 
   /// \brief Get the LLVM version string.
   std::string getLLVMVersionString() const {
     std::string VersionString = "Generated by llvm-cov -- llvm version ";
     VersionString += LLVM_VERSION_STRING;
     return VersionString;
   }
+
+  /// \brief Return the sub-directory name for file coverage reports.
+  static StringRef getCoverageDir() { return "coverage"; }
 };
 }
 
 #endif // LLVM_COV_COVERAGEVIEWOPTIONS_H

tools/llvm-cov/SourceCoverageView.h

protected:
   std::string getOutputPath(StringRef Path, StringRef Extension,
                             bool InToplevel, bool Relative = true) const;
 
   /// \brief If directory output is enabled, create a file in that directory
   /// at the path given by getOutputPath(). Otherwise, return stdout.
   Expected<OwnedStream> createOutputStream(StringRef Path, StringRef Extension,
                                            bool InToplevel) const;
 
-  /// \brief Return the sub-directory name for file coverage reports.
-  static StringRef getCoverageDir() { return "coverage"; }
-
 public:
   static std::unique_ptr<CoveragePrinter>
   create(const CoverageViewOptions &Opts);
 
   virtual ~CoveragePrinter() {}
 
   /// @name File Creation Interface
   /// @{
virtual void renderExpansionView(raw_ostream &OS, ExpansionView &ESV,
                                    unsigned ViewDepth) = 0;
 
   /// \brief Render an instantiation view and any nested views.
   virtual void renderInstantiationView(raw_ostream &OS, InstantiationView &ISV,
                                        unsigned ViewDepth) = 0;
 
   /// \brief Render the project title, the report title \p CellText and the
   /// created time for the view.
-  virtual void renderCellInTitle(raw_ostream &OS, StringRef CellText) = 0;
+  virtual void renderCellInTitle(raw_ostream &OS, StringRef CellText,
+                                 ArrayRef<StringRef> SourceFiles,
+                                 size_t Index) = 0;
 
   /// \brief Render the table header for a given source file.
   virtual void renderTableHeader(raw_ostream &OS, unsigned FirstUncoveredLineNo,
                                  unsigned IndentLevel) = 0;
 
   /// @}
 
   /// \brief Format a count using engineering notation with 3 significant
public:
 
   /// \brief Add a function instantiation subview to this view.
   void addInstantiation(StringRef FunctionName, unsigned Line,
                         std::unique_ptr<SourceCoverageView> View);
 
   /// \brief Print the code coverage information for a specific portion of a
   /// source file to the output stream.
   void print(raw_ostream &OS, bool WholeFile, bool ShowSourceName,
-             unsigned ViewDepth = 0);
+             unsigned ViewDepth = 0,
+             ArrayRef<StringRef> SourceFiles = ArrayRef<StringRef>(),
+             size_t Index = 0);
 };
 
 } // namespace llvm
 
 #endif // LLVM_COV_SOURCECOVERAGEVIEW_H

tools/llvm-cov/SourceCoverageView.cpp

std::string CoveragePrinter::getOutputPath(StringRef Path, StringRef Extension,
   assert(Extension.size() && "The file extension may not be empty");
 
   SmallString<256> FullPath;
 
   if (!Relative)
     FullPath.append(Opts.ShowOutputDirectory);
 
   if (!InToplevel)
-    sys::path::append(FullPath, getCoverageDir());
+    sys::path::append(FullPath, Opts.getCoverageDir());
 
   SmallString<256> ParentPath = sys::path::parent_path(Path);
   sys::path::remove_dots(ParentPath, /*remove_dot_dots=*/true);
   sys::path::append(FullPath, sys::path::relative_path(ParentPath));
 
   auto PathFilename = (sys::path::filename(Path) + "." + Extension).str();
   sys::path::append(FullPath, PathFilename);
   sys::path::native(FullPath);

 
 void SourceCoverageView::addInstantiation(
     StringRef FunctionName, unsigned Line,
     std::unique_ptr<SourceCoverageView> View) {
   InstantiationSubViews.emplace_back(FunctionName, Line, std::move(View));
 }
 
 void SourceCoverageView::print(raw_ostream &OS, bool WholeFile,
-                               bool ShowSourceName, unsigned ViewDepth) {
+                               bool ShowSourceName, unsigned ViewDepth,
+                               ArrayRef<StringRef> SourceFiles, size_t Index) {
   if (WholeFile)
-    renderCellInTitle(OS, "Coverage Report");
+    renderCellInTitle(OS, "Coverage Report", SourceFiles, Index);
 
   renderViewHeader(OS);
 
   if (ShowSourceName)
     renderSourceName(OS, WholeFile);
 
   renderTableHeader(OS, getFirstUncoveredLineNo(), ViewDepth);
 

tools/llvm-cov/SourceCoverageViewHTML.h

class SourceCoverageViewHTML : public SourceCoverageView {
   void renderLineCoverageColumn(raw_ostream &OS,
                                 const LineCoverageStats &Line) override;
 
   void renderLineNumberColumn(raw_ostream &OS, unsigned LineNo) override;
 
   void renderRegionMarkers(raw_ostream &OS, CoverageSegmentArray Segments,
                            unsigned ViewDepth) override;
 
-  void renderCellInTitle(raw_ostream &OS, StringRef CellText) override;
+  void renderCellInTitle(raw_ostream &OS, StringRef CellText,
+                         ArrayRef<StringRef> SourceFiles,
+                         size_t Index) override;
 
   void renderTableHeader(raw_ostream &OS, unsigned FirstUncoveredLineNo,
                          unsigned IndentLevel) override;
 
+  /// \brief Return a relative filename from \p FirstFilename to \p
+  /// SecondFileName.
+  ///
+  /// Typical usage:
+  /// \code
+  ///  case 1:
+  ///   first input file1: /coverage/dir1/test.cpp
+  ///   seconds input file2: /coverage/dir1/dir2/test1.cpp
+  ///   return string: ../../../coverage/dir1/dir2/test1.cpp.html
+  ///  case 2:
+  ///   first input file1: /output/dir1/test.cpp
+  ///   seconds input file2: /output/dir1/test1.cpp
+  ///   return string: ../../../output/dir1/test1.cpp.html
+  ///  case 3:
+  ///   first input file1: /output/dir1/dir2/test.cpp
+  ///   seconds input file2: /output/dir1/test1.cpp
+  ///   return string: ../../../../output/dir1/test1.cpp.html
+  ///  case 4:
+  ///   first input file1: /coverage/dir1/test.cpp
+  ///   seconds input file2: index.html
+  ///   return string: ../../../index.html
+  /// \endcode
+  std::string getRelativePath(const StringRef FirstFilename,
+                              const StringRef SecondFileName);
+
+  /// \brief Render the page navigation in the project summary.
+  void renderPageNavigation(raw_ostream &OS, ArrayRef<StringRef> SourceFiles,
+                            unsigned Index);
+
 public:
   SourceCoverageViewHTML(StringRef SourceName, const MemoryBuffer &File,
                          const CoverageViewOptions &Options,
                          coverage::CoverageData &&CoverageInfo)
       : SourceCoverageView(SourceName, File, Options, std::move(CoverageInfo)) {
   }
 };
 
 } // namespace llvm
 
 #endif // LLVM_COV_SOURCECOVERAGEVIEWHTML_H

tools/llvm-cov/SourceCoverageViewHTML.cpp

void SourceCoverageViewHTML::renderInstantiationView(raw_ostream &OS,
                                                      InstantiationView &ISV,
                                                      unsigned ViewDepth) {
   OS << BeginExpansionDiv;
   ISV.View->print(OS, /*WholeFile=*/false, /*ShowSourceName=*/true, ViewDepth);
   OS << EndExpansionDiv;
 }
 
 void SourceCoverageViewHTML::renderCellInTitle(raw_ostream &OS,
-                                               StringRef CellText) {
+                                               StringRef CellText,
+                                               ArrayRef<StringRef> SourceFiles,
+                                               size_t Index) {
   if (getOptions().hasProjectTitle())
     OS << tag(ProjectTitleTag, escape(getOptions().ProjectTitle, getOptions()));
   OS << tag(ReportTitleTag, escape(CellText, getOptions()));
   if (getOptions().hasCreatedTime())
     OS << tag(CreatedTimeTag,
               escape(getOptions().CreatedTimeStr, getOptions()));
+
+  if (getOptions().hasOutputDirectory())
+    renderPageNavigation(OS, SourceFiles, Index);
 }
 
 void SourceCoverageViewHTML::renderTableHeader(raw_ostream &OS,
                                                unsigned FirstUncoveredLineNo,
                                                unsigned ViewDepth) {
   std::string SourceLabel;
   if (FirstUncoveredLineNo == 0) {
     SourceLabel = tag("td", tag("pre", "Source"));
   } else {
     std::string LinkTarget = "#L" + utostr(uint64_t(FirstUncoveredLineNo));
     SourceLabel =
         tag("td", tag("pre", "Source (" +
                                  a(LinkTarget, "jump to first uncovered line") +
                                  ")"));
   }
 
   renderLinePrefix(OS, ViewDepth);
   OS << tag("td", tag("pre", "Line No.")) << tag("td", tag("pre", "Count"))
      << SourceLabel;
   renderLineSuffix(OS, ViewDepth);
 }
+
+std::string
+SourceCoverageViewHTML::getRelativePath(const StringRef FirstFilename,
+                                        const StringRef SecondFileName) {
+  // Resolve the dots in the file paths.
+  SmallString<256> FileOne(FirstFilename);
+  sys::path::remove_dots(FileOne, /* remove_dot_dot */ true);
+  SmallString<256> FileTwo(SecondFileName);
+  sys::path::remove_dots(FileTwo, /* remove_dot_dot */ true);
+  // HTML uses '/' as the separator
+#ifdef LLVM_ON_WIN32
+  std::replace(FileTwo.begin(), FileTwo.end(), '\\', '/');
+#endif
+  std::string RelativePath;
+  for (auto PathChar : FileOne.str())
+    if (sys::path::is_separator(PathChar))
+      RelativePath += "../";
+  if (SecondFileName == "index.html")
+    RelativePath += SecondFileName.str();
+  else {
+    RelativePath += getOptions().getCoverageDir().str() + '/';
+    RelativePath += sys::path::relative_path(FileTwo).str() + ".html";
+  }
+  return RelativePath;
+}
+
+void SourceCoverageViewHTML::renderPageNavigation(
+    raw_ostream &OS, ArrayRef<StringRef> SourceFiles,
+    unsigned Index) {
+  assert(((Index >= 0) && (Index < SourceFiles.size())) &&
+         "Source file doesn't find");
+  StringRef SourceFileName = SourceFiles[Index];
+  std::string RelativePathToIndex =
+      getRelativePath(SourceFileName, "index.html");
+  StringRef PrevSourceFile;
+  if (Index > 0)
+    PrevSourceFile = SourceFiles[Index - 1];
+  StringRef NextSourceFile;
+  if (Index < (SourceFiles.size() - 1))
+    NextSourceFile = SourceFiles[Index + 1];
+
+  std::string RelativePathToPrev;
+  if (!PrevSourceFile.empty())
+    RelativePathToPrev = getRelativePath(SourceFileName, PrevSourceFile);
+  std::string RelativePathToNext;
+  if (!NextSourceFile.empty())
+    RelativePathToNext = getRelativePath(SourceFileName, NextSourceFile);
+  OS << a(RelativePathToIndex, "Index");
+  if (!RelativePathToPrev.empty())
+    OS << " | " << a(RelativePathToPrev, "Prev");
+  if (!RelativePathToNext.empty())
+    OS << " | " << a(RelativePathToNext, "Next");
+}

tools/llvm-cov/SourceCoverageViewText.h

class SourceCoverageViewText : public SourceCoverageView {
   void renderLineCoverageColumn(raw_ostream &OS,
                                 const LineCoverageStats &Line) override;
 
   void renderLineNumberColumn(raw_ostream &OS, unsigned LineNo) override;
 
   void renderRegionMarkers(raw_ostream &OS, CoverageSegmentArray Segments,
                            unsigned ViewDepth) override;
 
-  void renderCellInTitle(raw_ostream &OS, StringRef CellText) override;
+  void renderCellInTitle(raw_ostream &OS, StringRef CellText,
+                         ArrayRef<StringRef> SourceFiles,
+                         size_t Index) override;
 
   void renderTableHeader(raw_ostream &OS, unsigned FirstUncoveredLineNo,
                          unsigned IndentLevel) override;
 
 public:
   SourceCoverageViewText(StringRef SourceName, const MemoryBuffer &File,
                          const CoverageViewOptions &Options,
                          coverage::CoverageData &&CoverageInfo)
       : SourceCoverageView(SourceName, File, Options, std::move(CoverageInfo)) {
   }
 };
 
 } // namespace llvm
 
 #endif // LLVM_COV_SOURCECOVERAGEVIEWTEXT_H

tools/llvm-cov/SourceCoverageViewText.cpp

void SourceCoverageViewText::renderInstantiationView(raw_ostream &OS,
                                                      InstantiationView &ISV,
                                                      unsigned ViewDepth) {
   renderLinePrefix(OS, ViewDepth);
   OS << ' ';
   ISV.View->print(OS, /*WholeFile=*/false, /*ShowSourceName=*/true, ViewDepth);
 }
 
 void SourceCoverageViewText::renderCellInTitle(raw_ostream &OS,
-                                               StringRef CellText) {
+                                               StringRef CellText,
+                                               ArrayRef<StringRef> SourceFiles,
+                                               size_t Index) {
   if (getOptions().hasProjectTitle())
     getOptions().colored_ostream(OS, raw_ostream::CYAN)
         << getOptions().ProjectTitle << "\n";
 
   getOptions().colored_ostream(OS, raw_ostream::CYAN) << CellText << "\n";
 
   if (getOptions().hasCreatedTime())
     getOptions().colored_ostream(OS, raw_ostream::CYAN)
         << getOptions().CreatedTimeStr << "\n";
 }
 
 void SourceCoverageViewText::renderTableHeader(raw_ostream &, unsigned,
                                                unsigned) {}