This patch provides easy navigation to find the zero count lines, especially useful when the source file is very large.
Details
Diff Detail
Event Timeline
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 | 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 | 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 | 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? |
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.
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.
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.
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,
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.
Great, this is exactly what I had in mind! I'd like to request two minor changes --
tools/llvm-cov/SourceCoverageView.cpp | ||
---|---|---|
169 | 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 | ||
360 | I think this should _only_ happen when !FullyCovered. Having inactive text that says "Go to ..." could be distracting. |
Lgtm with two minor changes.
tools/llvm-cov/SourceCoverageView.cpp | ||
---|---|---|
85 | 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 | ||
362 | Link{Text,Target} shouldn't be computed until we're in the !FullyCovered case. |
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.