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

[Vedant Kumar via llvm-commits]

vsk added a comment.

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).


================
Comment at: test/tools/llvm-cov/showProjectSummary.cpp:39
@@ -38,2 +38,3 @@
 // HTML: <span>Created:
+// HTML_NAVIGATION: <a href='../../index.html'>Index</a> | Prev | Next<br>
 // HTML-FILE: <pre>Source: {{.*}}showProjectSummary.cpp (Binary: showProjectSummary.covmapping)</pre>
----------------
I don't think we should display "Prev" or "Next" if they aren't links.

================
Comment at: test/tools/llvm-cov/showProjectSummary.cpp:39
@@ -38,2 +38,3 @@
 // HTML: <span>Created:
+// HTML_NAVIGATION: <a href='../../index.html'>Index</a> | Prev | Next<br>
 // HTML-FILE: <pre>Source: {{.*}}showProjectSummary.cpp (Binary: showProjectSummary.covmapping)</pre>
----------------
vsk wrote:
> I don't think we should display "Prev" or "Next" if they aren't links.
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?


https://reviews.llvm.org/D24367