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

[Vedant Kumar via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D24367#541163, @MaggieYi wrote:

> In https://reviews.llvm.org/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.


https://reviews.llvm.org/D24367