[PATCH][RFC] llvm-cov HTML generation

Fri Mar 18 09:39:40 PDT 2016

Hi Harlan,

The latest patch seems to be missing three files:
showLineExecutionCountsHTML.cpp, showTemplateInstantiationsHTML.cpp and
CoverageCSS.inc. Otherwise LGTM, assuming you didn't change these files.

Thanks,
Maggie

On Thu, Mar 17, 2016 at 8:27 PM, Harlan Haskins <hhaskins at apple.com> wrote:

> I’ve attached an updated patch, and (separately) the binary files it
> references.
>
> I still have no idea how to properly attach them.
>
> — Harlan
>
>
>
>
> On Mar 17, 2016, at 11:20 AM, Harlan Haskins via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
> On Mar 17, 2016, at 5:35 AM, Ying Yi <maggieyi666 at gmail.com> wrote:
>
> Thanks for updating the patch.
>
> 1) >If you could help me isolate why that might happen, I’d appreciate it.
>
> The variable Filename in the function getOutputPathForFile is a Twine.
> Parts of the Twine are being ‘lost’ in the release build which causes the
> issue. From the comments in the Twine.h header, “A Twine is not intended
> for use directly and should not be stored ... Twines should only be used
> accepted as const references in arguments”.
>
> To solve the issue, you could change the code to:
>     auto Filename = (sys::path::filename(SourceFile) + "." +
> FileExt).str();
>
>
> Thanks! I’ve updated my patch to reflect that.
>
> 2) The two new tests failed since the lineExecutionCountsHTML.covmapping
> and templateInstantiationsHTML.covmapping files are missing.
>
>
> This is an issue. How can I embed those binary files in a patch?
>
> 3) Could you add a test for '-format=text' with '-output-dir=...'?
>
>
> Yep, I’ll just modify the existing lineExecutionCounts test to also run
> outputting to a directory and running the same FileCheck on that file
>
> 4) By the way, the patch includes an unnecessary change to the
> llvm-cov.cpp file.
>
>
> Fixed.
>
> Thanks,
>
> Maggie
>
>
> I’ll shoot an updated patch once the tests pass and I can figure out how
> to add this binary file…
>
> Thanks again,
> Harlan
>
> On Mon, Mar 14, 2016 at 6:11 PM, Harlan Haskins <hhaskins at apple.com>
> wrote:
>
>> I haven’t been able to reproduce this issue. Not quite sure why you’re
>> seeing it.
>>
>> If you could help me isolate why that might happen, I’d appreciate it.
>>
>> In the meantime, a small final patch to silence that switch warning and
>> avoid some unnecessary copies.
>>
>>
>>
>> Thanks,
>> Harlan
>>
>> On Mar 11, 2016, at 1:14 AM, Ying Yi <maggieyi666 at gmail.com> wrote:
>>
>> These are the steps that I used to get the issue:
>>
>> 1) Build:
>>    $ cmake -G "Unix Makefiles"
>> -DCMAKE_INSTALL_PREFIX=prefix=~/upstream/bin  -DCMAKE_BUILD_TYPE=Release
>> -DLLVM_ENABLE_ASSERTIONS=On ~/upstream/llvm
>>    $ make
>>
>> 2) Test:
>>    $ cat test.cpp
>>    int main(int argc, char ** argv) {
>>       int x = 1;
>>
>>       for (int i = 0; i < 1000; ++i)
>>           x *= 2+1;
>>
>>       return x;
>>    }
>>
>>   $ ~/upstream/build/bin/clang -O0 -fprofile-instr-generate=test.profraw
>> -fcoverage-mapping "test.cpp"
>>   $ ./a.out
>>   $ ~/upstream/build/bin/llvm-profdata merge test.profraw -o
>> merged.profdata
>>   $ ~/upstream/build/bin/llvm-cov show -format=html -output-dir=./report
>> a.out -instr-profile=merged.profdata
>>
>>
>> Thanks,
>>
>>
>>
>> On Thu, Mar 10, 2016 at 10:41 PM, Harlan Haskins <hhaskins at apple.com>
>> wrote:
>>
>>>
>>> On Mar 10, 2016, at 1:30 PM, Ying Yi <maggieyi666 at gmail.com> wrote:
>>>
>>> Thanks for your reply and the updated patch.
>>>
>>> 1) When I use the release build of llvm-cov to generate HTML files, all
>>> the HTML files are named “html”. This issue also exists on UNIX.
>>>
>>> I haven’t seen this happen. Can you provide some steps to reproduce?
>>> Thanks!
>>>
>>> 2) The following warning is given when building llvm-cov using Visual
>>> Studio 2013.
>>>
>>> “sourcecoverageview.cpp (108): warning C4715:
>>> 'llvm::SourceCoverageView::create' : not all control paths return a value
>>> ”
>>>
>>> That warning is incorrect. It’s a switch statement over both cases of an
>>> enum — I want this because if we add another format, say, OFJSON or
>>> something, I want a compiler warning telling me the switch case isn’t
>>> exhaustive.
>>>
>>> Here’s an updated patch with some small sanity tests added:
>>>
>>>
>>>
>>> Best,
>>> Harlan
>>>
>>>
>>> On Wed, Mar 9, 2016 at 12:50 AM, Harlan Haskins <hhaskins at apple.com>
>>> wrote:
>>>
>>>> Updated patch with unistd removed, utf-8, and `console` replaced with
>>>> `text`.
>>>>
>>>> Thanks!
>>>> Harlan
>>>>
>>>>
>>>> On Mar 8, 2016, at 10:55 AM, Justin Bogner <mail at justinbogner.com>
>>>> wrote:
>>>>
>>>> Harlan Haskins <hhaskins at apple.com> writes:
>>>>
>>>> Thanks again! Responses inline.
>>>>
>>>> On Mar 8, 2016, at 5:08 AM, Ying Yi <maggieyi666 at gmail.com> wrote:
>>>>
>>>> Thanks for adding the index page. I have some comments:
>>>>
>>>> 1) The ‘unistd.h’ header won’t build on windows.
>>>>
>>>> Thanks for catching that — I was using it to debug, but don’t need
>>>> it. I also don’t have a Windows machine to debug, so thank you for
>>>> investigating portability!
>>>>
>>>> 2) The help text for ‘-format’ is different from llvm-profdata.exe
>>>> tool, which uses the following style:
>>>>    -format=<html|console>                 - Format to output comparison
>>>> ….
>>>>    Is there a standard way to do this?
>>>>
>>>> LLVM’s CommandLine library renders the help text. Also, I noticed I
>>>> used ‘comparison’ instead of ‘coverage’. Thanks!
>>>>
>>>> 3) Should ‘console’ be changed to ‘text’, because it doesn’t show in
>>>> the console when you give an output directory?
>>>>
>>>> Yeah, I think it could. I’ve cc’d Justin for his input — he originally
>>>> suggested Console.
>>>>
>>>>
>>>> My only problem with "text" is that this includes ANSI escape sequences
>>>> when it uses colour, so text seems a bit misleading. That said, it
>>>> doesn't matter much - I'm fine with either.
>>>>
>>>> 4) Could you please use an html charset that supports UTF-8?
>>>>
>>>> Yep. Thanks for catching that!
>>>>
>>>> 5) The HTML report can look quite different to the source file if it
>>>> includes tabs. Could you specify a smaller tab-size in the CSS file?
>>>>
>>>> That’s a good question, and I don’t quite know the correct answer to
>>>> it. GitHub has a complicated set of rules for tab sizing that makes a
>>>> ‘best guess’ based on the source file type. I wonder if it’s a good
>>>> candidate for a command line switch?
>>>>
>>>> In the future (not for this patch):
>>>>
>>>> 6) Could we support a user CSS file?
>>>> Currently, the CSS rules are embedded into the html page. Could
>>>> llvm-cov provide an option to use an external CSS file? This would
>>>> give the user more flexibility.
>>>>
>>>> I agree that it’d give the user more flexibility, but it’d be
>>>> difficult to do given the nature of the directory structure. Maybe, if
>>>> you know you’re gonna be running it on a server where the ‘root’ is
>>>> well-defined, we could just dump the css file to disk and put a
>>>> relative <link> tag in. But if it’s gonna be ten directories down,
>>>> we’ll need to have some logic in that replaces all the parent paths in
>>>> the link with /../..’s to make the file resolve. I think it can be
>>>> done, just with more input from the user about their intention with
>>>> hosting.
>>>>
>>>> 7) Could we put the functions/lines summary information into the
>>>> HTML files? As a rough example:
>>>>
>>>> Lines: 9/10 (90%)
>>>> Functions: 1/1 (100%)
>>>>
>>>> I think this totally can be done — it’s out of scope for this patch
>>>> (it’s already a pretty big patch as it is.)
>>>> Some kind of merging of ‘show’ and ‘report’ would be ideal. Maybe the
>>>> index page has at-a-glance information about each file?
>>>>
>>>>
>>>> Yep, this sounds like a great idea, but it's probably more suited to a
>>>> follow up patch.
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Ying Yi
>>> SN Systems Ltd - Sony Computer Entertainment Group.
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Ying Yi
>> SN Systems Ltd - Sony Computer Entertainment Group.
>>
>>
>>
>>
>
>
> --
> Ying Yi
> SN Systems Ltd - Sony Computer Entertainment Group.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>

-- 
Ying Yi
SN Systems Ltd - Sony Computer Entertainment Group.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160318/f6eded18/attachment-0001.html>