[PATCH][RFC] llvm-cov HTML generation
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 15:55:27 PDT 2016
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