[PATCH][RFC] llvm-cov HTML generation
Harlan Haskins via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 12:46:20 PDT 2016
Should I abandon and re-create the review?
Sent from my iPhone
> On Mar 23, 2016, at 9:42 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
> Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> http://reviews.llvm.org/D18278
>>
>> This should be better.
>
> It looks like this review never actually made it to the list, so any
> review that happened there hasn't shown up. Phabricator's really bad for
> doing the wrong thing unless you follow the steps to create a review
> very carefully:
>
> http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
>
> Specifically, if you don't add llvm-commits when you create the review,
> the list never gets the patch, and there's apparently no way to rectify
> this other than abandoning and re-creating the review.
>
>> Thanks!
>>
>> — Harlan
>>
>> On Mar 18, 2016, at 9:39 AM, Ying Yi <maggieyi666 at gmail.com> wrote:
>>
>> 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.
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list