Page MenuHomePhabricator

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

Authored by MaggieYi on Sep 8 2016, 4:38 PM.

Details

Reviewers
davidxl
bogner
vsk
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.

Diff Detail

Event Timeline

MaggieYi updated this revision to Diff 70763.Sep 8 2016, 4:38 PM
MaggieYi retitled this revision from to [llvm-cov] Provided easy navigation to find the previous and next report..
MaggieYi updated this object.
MaggieYi added a reviewer: vsk.
MaggieYi added a subscriber: llvm-commits.
vsk edited edge metadata.Sep 9 2016, 3:08 PM

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 updated this revision to Diff 71164.Sep 13 2016, 7:40 AM
MaggieYi edited edge metadata.
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 marked 2 inline comments as done.Sep 13 2016, 7:42 AM
MaggieYi added inline comments.
test/tools/llvm-cov/showProjectSummary.cpp
35

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

vsk added a comment.Sep 13 2016, 10:31 AM
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 abandoned this revision.Sep 19 2016, 4:28 AM
MaggieYi marked an inline comment as done.

I agree with you and abandon this patch.