[PATCH][RFC] llvm-cov HTML generation
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 12:56:55 PDT 2016
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?
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160324/8389b659/attachment-0001.html>
More information about the llvm-commits
mailing list