[llvm-cov] Add the "Goto first zero count" feature.

[MaggieYi]

Summary

This patch provides easy navigation to find the zero count lines, especially useful when the source file is very large.

[vsk]

On a high-level, the changes you've made look good. However, I think that this
patch needs to be split up into 3 smaller patches for each functional change
you listed in your summary. That would make it easier for me to review it, and
would make the commit history easier to read through.

I've included a few inline comments which might help you split the code up.

tools/llvm-cov/SourceCoverageView.h

262 ↗

(On Diff #67208)

The two implementations of renderProjectSummary() are very similar. Could you pull the code into SourceCoverageView, and modify renderViewHeader() to account for any differences (e.g, by passing bool WholeFile into it)?

Once this is done, please go through the pure virtual methods you've added and eliminate the ones which are not directly called by the SourceCoverageView class.

268 ↗

(On Diff #67208)

I don't think it makes sense to have a method called 'renderJavaScriptHeader' in generic presentation code. It would make more sense to me to sink this logic into another method, e.g renderViewFooter().

tools/llvm-cov/SourceCoverageViewHTML.cpp

362 ↗

(On Diff #67208)

I'd rather not rely on file extension naming conventions on getting this right. Could you modify createFunctionView() to sneak an indicator bit into SourceCoverageView?

[MaggieYi]

In D23277#508994, @vsk wrote:

On a high-level, the changes you've made look good. However, I think that this
patch needs to be split up into 3 smaller patches for each functional change
you listed in your summary. That would make it easier for me to review it, and
would make the commit history easier to read through.

Many thanks for your comments. I am happy to split up this patch into 3 smaller patches.
I have put the first patch into Phabricator (https://reviews.llvm.org/D23281).

I will update the rest of patch following your comments.

[MaggieYi]

I have put the change of adding the project summary into Phabricator (https://reviews.llvm.org/D23345).

The patch has been updated for only adding the "Goto first zero count" feature.

[MaggieYi]

Updating the patch to support the "Goto first zero count" feature.

[vsk]

Thanks for splitting things up :).

I'm not sure that the javascript bit is needed. It looks like a way to remove/disable a link, but I don't see any issue with leaving the link active.

I also don't think we need a 'zeroline' or 'zero' id. Every line already has an anchor: <a name='L$N'>. I think a "Jump to the first uncovered line" link which uses one of the line anchors would do the job.

[MaggieYi]

Reused the existing native_separator.c test to check fully covered source file case.

[MaggieYi]

In D23277#526594, @vsk wrote:

Thanks for splitting things up :).

I'm not sure that the javascript bit is needed. It looks like a way to remove/disable a link, but I don't see any issue with leaving the link active.

If the link is removed, the user could easily know that this source file is fully covered. From this point, it is better to remove the link.

In addition, the JavaScript is also used by another patch. Following this patch, I want to submit another patch which provides 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. The JavaScript of removing the link is also needed by the index/prev/next patch.

I also don't think we need a 'zeroline' or 'zero' id. Every line already has an anchor: <a name='L$N'>. I think a "Jump to the first uncovered line" link which uses one of the line anchors would do the job.

I use the following way to implement “Goto first zero line” feature.

To create links to jump to the first uncovered line within the page, two HTML tags need to be used.

<a href="#zero">Goto first zero count</a>

This first tag is almost the same as any other HTML tag you would use to create a link to another page. However, to create a bookmark (anchor), I must start the link with a #, which represents a name statement, used in the next tag. When the user clicks on the "Goto first zero count" link, the computer would then go to the name tag, if found on the same page of the link.

id="zero"

This next piece of code is where the link will go to on the page when the link is clicked. The ID attribute is added to a <tr> tag.

<tr id="zero">

Since I also need an ID to remove the link by Javascript, the 'id="zeroline"'has been added to the first tag.

<a href="#zero" 'id="zeroline">Goto first zero count</a>

Both 'zeroline' and 'zero' are needed using this method. I am not sure the way you mentioned here. Could you please explain it by an example?

Thanks,

[vsk]

In D23277#529763, @MaggieYi wrote:

In D23277#526594, @vsk wrote:

Thanks for splitting things up :).

I'm not sure that the javascript bit is needed. It looks like a way to remove/disable a link, but I don't see any issue with leaving the link active.

If the link is removed, the user could easily know that this source file is fully covered. From this point, it is better to remove the link.

If the source file is fully covered, we shouldn't emit the link. I don't understand why it's necessary to emit the link, and then add code to disable it when the page is viewed.

In addition, the JavaScript is also used by another patch. Following this patch, I want to submit another patch which provides 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. The JavaScript of removing the link is also needed by the index/prev/next patch.

Similarly, if there is no "next" report, a next link shouldn't be emitted. Ditto for "prev" links.

I also don't think we need a 'zeroline' or 'zero' id. Every line already has an anchor: <a name='L$N'>. I think a "Jump to the first uncovered line" link which uses one of the line anchors would do the job.

I use the following way to implement “Goto first zero line” feature.

To create links to jump to the first uncovered line within the page, two HTML tags need to be used.

<a href="#zero">Goto first zero count</a>

This first tag is almost the same as any other HTML tag you would use to create a link to another page. However, to create a bookmark (anchor), I must start the link with a #, which represents a name statement, used in the next tag. When the user clicks on the "Goto first zero count" link, the computer would then go to the name tag, if found on the same page of the link.

id="zero"

This next piece of code is where the link will go to on the page when the link is clicked. The ID attribute is added to a <tr> tag.

<tr id="zero">

Since I also need an ID to remove the link by Javascript, the 'id="zeroline"'has been added to the first tag.

<a href="#zero" 'id="zeroline">Goto first zero count</a>

Both 'zeroline' and 'zero' are needed using this method. I am not sure the way you mentioned here. Could you please explain it by an example?

Sure, let's say that we determined that line 100 is the first line which has an execution count of 0. Then, we can emit: <a href="#L100">Go to first unexecuted line</a>. I understand how you're using #zero and #zeroline, but still don't think that they are essential.

[MaggieYi]

Thanks for your explanation. I have updated the patch following your advice.

[vsk]

Great, this is exactly what I had in mind! I'd like to request two minor changes --

tools/llvm-cov/SourceCoverageView.cpp

172 ↗

(On Diff #69989)

Can you move this logic into a private helper method, say: getFirstUncoveredLineNo(CoverageInfo);? Having it inline in print() makes it a bit hard to read. Also, you shouldn't need a line_iterator here. This could be:

const auto *MinSeg = std::min_element(CoverageInfo.begin(), CoverageInfo.end(),
  [](const coverage::CoverageSegment *L, const coverage::CoverageSegment *R) {
    // L is less than R if (1) it's an uncovered segment (has a 0 count), and
    // (2) either R is not an uncovered segment, or L has a lower line number than R
  });
if (MinSeg is really an uncovered segment)
  return the first uncovered line number;
llvm_unreachable(this file isn't supposed to be fully covered);

tools/llvm-cov/SourceCoverageViewHTML.cpp

356 ↗

(On Diff #69989)

I think this should _only_ happen when !FullyCovered. Having inactive text that says "Go to ..." could be distracting.

[MaggieYi]

Thanks for your pseudocode . Two issues have been fixed.

[vsk]

Lgtm with two minor changes.

tools/llvm-cov/SourceCoverageView.cpp

86 ↗

(On Diff #70228)

This makes a copy of the CoverageSegment. const coverage::CoverageSegment & should work just as well, without the extra copy. The comparator you defined for the call to min_element() should also take const references.

tools/llvm-cov/SourceCoverageViewHTML.cpp

358 ↗

(On Diff #70228)

Link{Text,Target} shouldn't be computed until we're in the !FullyCovered case.

llvm/trunk/test/tools/llvm-cov/native_separators.c

 // RUN: FileCheck -check-prefixes=HTML -input-file=%t.dir/coverage/tmp/native_separators.c.html %s
 
 // TEXT-INDEX: \tmp\native_separators.c
 // HTML-INDEX: >tmp\native_separators.c</a>
 // HTML: <pre>Source: \tmp\native_separators.c</pre>
 // HTML: tools\llvm-cov\Inputs\native_separators.covmapping</pre>
 
 int main() {}
+
+// Re-purpose this file to test that "Go to first unexecuted line" feature.
+
+// RUN: llvm-cov show %S/Inputs/native_separators.covmapping -instr-profile %t.profdata -filename-equivalence -format html -o %t.dir %s
+// RUN: FileCheck -input-file %t.dir/coverage/tmp/native_separators.c.html %s
+// CHECK-NOT: >Go to first unexecuted line<

llvm/trunk/test/tools/llvm-cov/showProjectSummary.cpp

 // TEXT: Code Coverage Report
 // TEXT: Created:
 // TEXT-FILE: showProjectSummary.cpp:
 // TEXT-FILE: showProjectSummary.covmapping:
 // TEXT-FUNCTION: main:
 
 // 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 -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
+// RUN: FileCheck -check-prefixes=HTML,HTML-FILE,HTML-HEADER,HTML-UNCOVEREDLINE -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 -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
+// RUN: FileCheck -check-prefixes=HTML-TITLE,HTML,HTML-FILE,HTML-HEADER,HTML-UNCOVEREDLINE -input-file %t.dir/coverage/tmp/showProjectSummary.cpp.html %s
 // RUN: FileCheck -check-prefixes=HTML-TITLE,HTML -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 -input-file %t.dir/functions.html %s
 // HTML-TITLE: <div class='project-title'>
 // HTML-TITLE: <span>Test Suite</span>
 // HTML: <div class='report-title'>
 // HTML: <span>Code Coverage Report</span>
 // HTML: <div class='created-time'>
 // HTML: <span>Created:
 // HTML-FILE: <pre>Source:
 // HTML-FILE: showProjectSummary.cpp</pre>
 // HTML-FILE: <pre>Binary:
 // HTML-FILE: showProjectSummary.covmapping</pre>
 // HTML-FUNCTION: <pre>Function: main</pre>
+// HTML-UNCOVEREDLINE: <a href='#L8'>Go to first unexecuted line</a>
 // HTML-HEADER: <tr><td><span><pre>Line No.</pre></span></td>
 // HTML-HEADER: <td><span><pre>Count</pre></span></td>
 // HTML-HEADER: <td><span><pre>Source</pre></span></td>

llvm/trunk/tools/llvm-cov/SourceCoverageView.h

class SourceCoverageView {
 
   /// A container for all instantiations (e.g template functions) in the source
   /// on display.
   std::vector<InstantiationView> InstantiationSubViews;
 
   /// Specifies whether or not the view is a function view.
   bool FunctionView;
 
+  /// Get the first uncovered line number for the source file.
+  unsigned getFirstUncoveredLineNo();
+
 protected:
   struct LineRef {
     StringRef Line;
     int64_t LineNo;
 
     LineRef(StringRef Line, int64_t LineNo) : Line(Line), LineNo(LineNo) {}
   };
 
   using CoverageSegmentArray = ArrayRef<const coverage::CoverageSegment *>;
 
   /// @name Rendering Interface
   /// @{
 
   /// \brief Render a header for the view.
   virtual void renderViewHeader(raw_ostream &OS) = 0;
 
   /// \brief Render a footer for the view.
   virtual void renderViewFooter(raw_ostream &OS) = 0;
 
   /// \brief Render the source name for the view.
-  virtual void renderSourceName(raw_ostream &OS, bool WholeFile) = 0;
+  virtual void renderSourceName(raw_ostream &OS, bool WholeFile,
+                                unsigned FirstUncoveredLineNo) = 0;
 
   /// \brief Render the line prefix at the given \p ViewDepth.
   virtual void renderLinePrefix(raw_ostream &OS, unsigned ViewDepth) = 0;
 
   /// \brief Render the line suffix at the given \p ViewDepth.
   virtual void renderLineSuffix(raw_ostream &OS, unsigned ViewDepth) = 0;
 
   /// \brief Render a view divider at the given \p ViewDepth.

llvm/trunk/tools/llvm-cov/SourceCoverageView.cpp

CoveragePrinter::create(const CoverageViewOptions &Opts) {
   case CoverageViewOptions::OutputFormat::Text:
     return llvm::make_unique<CoveragePrinterText>(Opts);
   case CoverageViewOptions::OutputFormat::HTML:
     return llvm::make_unique<CoveragePrinterHTML>(Opts);
   }
   llvm_unreachable("Unknown coverage output format!");
 }
 
+unsigned SourceCoverageView::getFirstUncoveredLineNo() {
+  auto CheckIfUncovered = [](const coverage::CoverageSegment &S) {
+    return S.HasCount && S.Count == 0;
+  };
+  // L is less than R if (1) it's an uncovered segment (has a 0 count), and (2)
+  // either R is not an uncovered segment, or L has a lower line number than R.
+  const auto MinSegIt =
+      std::min_element(CoverageInfo.begin(), CoverageInfo.end(),
+                       [CheckIfUncovered](const coverage::CoverageSegment &L,
+                                          const coverage::CoverageSegment &R) {
+                         return (CheckIfUncovered(L) &&
+                                 (!CheckIfUncovered(R) || (L.Line < R.Line)));
+                       });
+  if (CheckIfUncovered(*MinSegIt))
+    return (*MinSegIt).Line;
+  // There is no uncovered line, return zero.
+  return 0;
+}
+
 std::string SourceCoverageView::formatCount(uint64_t N) {
   std::string Number = utostr(N);
   int Len = Number.size();
   if (Len <= 3)
     return Number;
   int IntLen = Len % 3 == 0 ? 3 : Len % 3;
   std::string Result(Number.data(), IntLen);
   if (IntLen != 3) {

 
 void SourceCoverageView::print(raw_ostream &OS, bool WholeFile,
                                bool ShowSourceName, unsigned ViewDepth) {
   if (WholeFile)
     renderCellInTitle(OS, "Code Coverage Report");
 
   renderViewHeader(OS);
 
+  unsigned FirstUncoveredLineNo = 0;
+  if (WholeFile)
+    FirstUncoveredLineNo = getFirstUncoveredLineNo();
+
   if (ShowSourceName)
-    renderSourceName(OS, WholeFile);
+    renderSourceName(OS, WholeFile, FirstUncoveredLineNo);
 
   renderTableHeader(OS, ViewDepth);
   // We need the expansions and instantiations sorted so we can go through them
   // while we iterate lines.
   std::sort(ExpansionSubViews.begin(), ExpansionSubViews.end());
   std::sort(InstantiationSubViews.begin(), InstantiationSubViews.end());
   auto NextESV = ExpansionSubViews.begin();
   auto EndESV = ExpansionSubViews.end();

llvm/trunk/tools/llvm-cov/SourceCoverageViewHTML.h

 };
 
 /// \brief A code coverage view which supports html-based rendering.
 class SourceCoverageViewHTML : public SourceCoverageView {
   void renderViewHeader(raw_ostream &OS) override;
 
   void renderViewFooter(raw_ostream &OS) override;
 
-  void renderSourceName(raw_ostream &OS, bool WholeFile) override;
+  void renderSourceName(raw_ostream &OS, bool WholeFile,
+                        unsigned FirstUncoveredLineNo) override;
 
   void renderLinePrefix(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderLineSuffix(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderViewDivider(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderLine(raw_ostream &OS, LineRef L,

llvm/trunk/tools/llvm-cov/SourceCoverageViewHTML.cpp

 void SourceCoverageViewHTML::renderViewHeader(raw_ostream &OS) {
   OS << LineBreak << BeginCenteredDiv << BeginTable;
 }
 
 void SourceCoverageViewHTML::renderViewFooter(raw_ostream &OS) {
   OS << EndTable << EndCenteredDiv;
 }
 
-void SourceCoverageViewHTML::renderSourceName(raw_ostream &OS, bool WholeFile) {
+void SourceCoverageViewHTML::renderSourceName(raw_ostream &OS, bool WholeFile,
+                                              unsigned FirstUncoveredLineNo) {
   OS << BeginSourceNameDiv;
   // Render the source name for the view.
   std::string SourceFile = isFunctionView() ? "Function: " : "Source: ";
   SourceFile += getSourceName().str();
   SmallString<128> SourceText(SourceFile);
   sys::path::remove_dots(SourceText, /*remove_dot_dots=*/true);
   sys::path::native(SourceText);
   OS << tag("pre", escape(SourceText, getOptions()));
+  if (WholeFile) {
     // Render the object file name for the view.
-  if (WholeFile)
     OS << tag("pre",
               escape("Binary: " + getOptions().ObjectFilename, getOptions()));
+    // Render the "Go to first unexecuted line" link for the view.
+    if (FirstUncoveredLineNo != 0) { // The file is not fully covered
+      std::string LinkText =
+          escape("Go to first unexecuted line", getOptions());
+      std::string LinkTarget = "#L" + utostr(uint64_t(FirstUncoveredLineNo));
+      OS << tag("pre", a(LinkTarget, LinkText));
+    }
+  }
   OS << EndSourceNameDiv;
 }
 
 void SourceCoverageViewHTML::renderLinePrefix(raw_ostream &OS, unsigned) {
   OS << "<tr>";
 }
 
 void SourceCoverageViewHTML::renderLineSuffix(raw_ostream &OS, unsigned) {

llvm/trunk/tools/llvm-cov/SourceCoverageViewText.h

 };
 
 /// \brief A code coverage view which supports text-based rendering.
 class SourceCoverageViewText : public SourceCoverageView {
   void renderViewHeader(raw_ostream &OS) override;
 
   void renderViewFooter(raw_ostream &OS) override;
 
-  void renderSourceName(raw_ostream &OS, bool WholeFile) override;
+  void renderSourceName(raw_ostream &OS, bool WholeFile,
+                        unsigned FirstUncoveredLineNo) override;
 
   void renderLinePrefix(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderLineSuffix(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderViewDivider(raw_ostream &OS, unsigned ViewDepth) override;
 
   void renderLine(raw_ostream &OS, LineRef L,

llvm/trunk/tools/llvm-cov/SourceCoverageViewText.cpp

 }
 
 } // anonymous namespace
 
 void SourceCoverageViewText::renderViewHeader(raw_ostream &) {}
 
 void SourceCoverageViewText::renderViewFooter(raw_ostream &) {}
 
-void SourceCoverageViewText::renderSourceName(raw_ostream &OS, bool WholeFile) {
+void SourceCoverageViewText::renderSourceName(raw_ostream &OS, bool WholeFile,
+                                              unsigned FirstUncoveredLineNo) {
   getOptions().colored_ostream(OS, raw_ostream::CYAN) << getSourceName()
                                                       << ":\n";
   if (WholeFile) {
     getOptions().colored_ostream(OS, raw_ostream::CYAN)
         << getOptions().ObjectFilename << ":\n";
   }
 }