[PATCH][RFC] llvm-cov HTML generation
Harlan Haskins via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 16:01:34 PDT 2016
I just made a small update.
> On Mar 24, 2016, at 3:55 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
> Xinliang David Li <xinliangli at gmail.com> writes:
>> Unless there is a way to import the lost review history (only a tiny part
>> of it is lost), I don't see the advantage of creating a new one. Can you
>> just modify that review with llvm-commits@ added as subscriber?
>
> After you do that you'll have to update the patch so that it actually
> mails a copy to the list, I think.
>
>> David
>>
>> On Thu, Mar 24, 2016 at 12:46 PM, Harlan Haskins via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> 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
>>> _______________________________________________
>>> 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