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