Page MenuHomePhabricator

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

Authored by MaggieYi on Aug 8 2016, 11:38 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 67208.Aug 8 2016, 11:38 AM
MaggieYi retitled this revision from to Improves the HTML report for the source file..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, bogner.
MaggieYi added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 8 2016, 1:11 PM

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?

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 updated this revision to Diff 67510.Aug 10 2016, 5:20 AM
MaggieYi retitled this revision from Improves the HTML report for the source file. to [llvm-cov] Add the "Goto first zero count" feature..
MaggieYi updated this object.
MaggieYi edited edge metadata.

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 updated this revision to Diff 69167.Aug 24 2016, 2:02 PM

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

vsk added a comment.Aug 26 2016, 11:25 AM

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 updated this revision to Diff 69772.Aug 30 2016, 3:58 PM

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

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 added a comment.Aug 30 2016, 5:22 PM
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 updated this revision to Diff 69989.Sep 1 2016, 4:39 AM

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

vsk added a comment.Sep 1 2016, 10:58 AM

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 updated this revision to Diff 70228.Sep 2 2016, 2:00 PM

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

vsk accepted this revision.Sep 2 2016, 2:15 PM
vsk edited edge metadata.

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.

This revision is now accepted and ready to land.Sep 2 2016, 2:15 PM
This revision was automatically updated to reflect the committed changes.