[PATCH] D24687: [llvm-cov] Add a project coverage summary above the index page.

[Ying Yi via llvm-commits]

Hi Vedant,


Thanks for your explanation.


> Based on your sample report, I think the second change (repeating the
summary
statistics) is the only one that makes the report easier to read. This can
be
implemented with a one-line change.


I would prefer to keep the current index report rather than moving/copying
the total table row to the top of the index table. So I have decided not to
make any changes.

I will abandon the D24687 Phabricator review.


Thanks,

On Thu, Sep 22, 2016 at 2:17 AM, Vedant Kumar <vsk at apple.com> wrote:

> There are three changes here: display the longest common prefix of the
> filenames, repeat the summary statistics at the top of the page, and make
> the
> filename column only display one path component (leaving the rest to
> another
> column).
>
> Based on your sample report, I think the second change (repeating the
> summary
> statistics) is the only one that makes the report easier to read. This can
> be
> implemented with a one-line change.
>
> I don't think the other two changes contribute useful new information, or
> make
> reports easier to read. Knowing the precise location of the source
> directory
> doesn't matter when you're looking at the coverage for some file. This
> information is just noise if the source lives on a bot. Similarly, the new
> file
> directory column contains visually indistinct and redundant entries.
>
> I would rather not move forward with those two changes.
>
> thanks,
> vedant
>
> > On Sep 21, 2016, at 10:48 AM, Ying Yi <maggieyi666 at gmail.com> wrote:
> >
> > Hi Vedant,
> >
> >
> >
> > I have attached the index.html page which includes the project directory
> in the summary, and separate columns for files and directories in the index
> table. Listing the project directory in summary allows user to construct
> the full file path.
> >
> >
> >
> > For example:
> >
> > Project directory: I:\svn\PS4\toolchain-test\tests\branches\PS4_4.500\
> benchmarks
> > File directory: \Bullet-2.76\Demos\Benchmarks
> >  The file BenchmarkDemo.cpp could find in the folder:
> >  I:\svn\PS4\toolchain-test\tests\branches\PS4_4.500\
> benchmarks\Bullet-2.76\Demos\Benchmarks .
> >
> > Thanks,
> >
> >
> >
> > On Tue, Sep 20, 2016 at 10:03 PM, Vedant Kumar <vsk at apple.com> wrote:
> > vsk added a comment.
> >
> > > If the project is branched, the longest-common prefix of the path (I
> called the project directory) will show the branch name. The project
> directory will tell the user where the source code come from.
> >
> >
> > It would be easier for me to understand the motivation behind this if
> you could upload an example report with your changes. I still don't
> understand what it is about the current scheme that makes it difficult for
> readers to figure out where some source file comes from.
> >
> >
> > ================
> > Comment at: tools/llvm-cov/SourceCoverageViewHTML.cpp:431
> > @@ -390,1 +430,3 @@
> > +  OSRef << BeginCenteredDiv << BeginTable;
> > +  emitColumnLabelsForIndex(OSRef);
> >    for (unsigned I = 0, E = FileReports.size(); I < E; ++I)
> > ----------------
> > This patch still seems larger than it needs to be. Can't we get the
> entire desired functional change by just inserting another
> `emitFileSummary(OSRef, "Totals", Totals, /*IsTotals=*/true)` here?
> >
> >
> > https://reviews.llvm.org/D24687
> >
> >
> >
> >
> >
> >
> > --
> > Ying Yi
> > SN Systems - Sony Interactive Entertainment
> > <html_index.zip>
>
>


-- 
Ying Yi
SN Systems - Sony Interactive Entertainment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160926/ac81feb5/attachment.html>