<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 17, 2016, at 11:20 AM, Harlan Haskins via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 17, 2016, at 5:35 AM, Ying Yi <<a href="mailto:maggieyi666@gmail.com" class="">maggieyi666@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Thanks for updating the patch.<br class=""><br class="">1) >If you could help me isolate why that might happen, I’d appreciate it.<br class=""><br class="">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”.<br class=""><br class="">To solve the issue, you could change the code to:<br class="">    auto Filename = (sys::path::filename(SourceFile) + "." + FileExt).str();<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Thanks! I’ve updated my patch to reflect that.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">2) The two new tests failed since the lineExecutionCountsHTML.covmapping and templateInstantiationsHTML.covmapping files are missing. <br class=""></div></div></blockquote><div class=""><br class=""></div>This is an issue. How can I embed those binary files in a patch?<br class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">3) Could you add a test for '-format=text' with '-output-dir=...'? <br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Yep, I’ll just modify the existing lineExecutionCounts test to also run outputting to a directory and running the same FileCheck on that file</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">4) By the way, the patch includes an unnecessary change to the llvm-cov.cpp file.<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">Fixed.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">Thanks,<br class=""><br class="">Maggie<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">I’ll shoot an updated patch once the tests pass and I can figure out how to add this binary file…</div><div class=""><br class=""></div><div class="">Thanks again,</div><div class="">Harlan</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 14, 2016 at 6:11 PM, Harlan Haskins <span dir="ltr" class=""><<a href="mailto:hhaskins@apple.com" target="_blank" class="">hhaskins@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">I haven’t been able to reproduce this issue. Not quite sure why you’re seeing it.<div class=""><br class=""></div><div class="">If you could help me isolate why that might happen, I’d appreciate it.</div><div class=""><br class=""></div><div class="">In the meantime, a small final patch to silence that switch warning and avoid some unnecessary copies.</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Harlan</div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 11, 2016, at 1:14 AM, Ying Yi <<a href="mailto:maggieyi666@gmail.com" target="_blank" class="">maggieyi666@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">These are the steps that I used to get the issue:<br class=""><br class="">1) Build:<br class="">   $ cmake -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=prefix=~/upstream/bin  -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On ~/upstream/llvm<br class="">   $ make<br class=""><br class="">2) Test:<br class="">   $ cat test.cpp<br class="">   int main(int argc, char ** argv) {
<br class="">      int x = 1;
<br class=""> <br class="">      for (int i = 0; i < 1000; ++i)
<br class="">          x *= 2+1;
<br class="">  <br class="">      return x;
<br class="">   }<br class=""><br class="">  $ ~/upstream/build/bin/clang -O0 -fprofile-instr-generate=test.profraw -fcoverage-mapping "test.cpp"<br class="">  $ ./a.out<br class="">  $ ~/upstream/build/bin/llvm-profdata merge test.profraw -o merged.profdata<br class="">  $ ~/upstream/build/bin/llvm-cov show -format=html -output-dir=./report a.out -instr-profile=merged.profdata<br class=""><br class=""><br class="">Thanks,<br class=""><br class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Mar 10, 2016 at 10:41 PM, Harlan Haskins <span dir="ltr" class=""><<a href="mailto:hhaskins@apple.com" target="_blank" class="">hhaskins@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Mar 10, 2016, at 1:30 PM, Ying Yi <<a href="mailto:maggieyi666@gmail.com" target="_blank" class="">maggieyi666@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><p class="MsoNormal"><span style="font-size:10pt;line-height:15.3333px" class="">Thanks for your reply and the updated patch.</span></p></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><p class="MsoNormal"><span style="font-size:10pt;line-height:15.3333px" class="">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.</span></p></div></div></blockquote></span>I haven’t seen this happen. Can you provide some steps to reproduce? Thanks!<span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><p class="MsoNormal"><span style="font-size:10pt;line-height:15.3333px" class="">2) The following warning is given when building llvm-cov using Visual Studio 2013.</span></p><p class="MsoNormal"><span style="font-size:10pt;line-height:15.3333px;color:rgb(30,30,30);background-image:none;background-color:white;background-repeat:repeat repeat" class="">“sourcecoverageview.cpp (108): warning C4715: 'llvm::SourceCoverageView::create' : not all control paths return a value</span><span style="font-size:10pt;line-height:15.3333px;color:rgb(30,30,30)" class="">”</span></p></div></div></blockquote></span>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.</div><div class=""><br class=""></div><div class="">Here’s an updated patch with some small sanity tests added:</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div><div class=""><br class=""></div><div class="">Best,</div><div class="">Harlan</div><div class=""><br class=""><blockquote type="cite" class=""><div class="gmail_extra" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br class=""><div class="gmail_quote">On Wed, Mar 9, 2016 at 12:50 AM, Harlan Haskins<span class=""> </span><span dir="ltr" class=""><<a href="mailto:hhaskins@apple.com" target="_blank" class="">hhaskins@apple.com</a>></span><span class=""> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class="">Updated patch with unistd removed, utf-8, and `console` replaced with `text`.<div class=""><br class=""></div><div class="">Thanks!</div><span class=""><font color="#888888" class=""><div class="">Harlan<br class=""><div class=""></div></div></font></span></div><br class=""><div style="word-wrap:break-word" class=""><div class=""><div class=""></div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 8, 2016, at 10:55 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank" class="">mail@justinbogner.com</a>> wrote:</div><br class=""><div class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">Harlan Haskins <</span><a href="mailto:hhaskins@apple.com" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank" class="">hhaskins@apple.com</a><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">> writes:</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">Thanks again! Responses inline.<br class=""><blockquote type="cite" class="">On Mar 8, 2016, at 5:08 AM, Ying Yi <<a href="mailto:maggieyi666@gmail.com" target="_blank" class="">maggieyi666@gmail.com</a>> wrote:<br class=""><br class="">Thanks for adding the index page. I have some comments:<br class=""><br class="">1) The ‘unistd.h’ header won’t build on windows.<br class=""><br class=""></blockquote>Thanks for catching that — I was using it to debug, but don’t need<br class="">it. I also don’t have a Windows machine to debug, so thank you for<br class="">investigating portability!<br class=""><br class=""><blockquote type="cite" class="">2) The help text for ‘-format’ is different from llvm-profdata.exe<br class="">tool, which uses the following style:<br class="">   -format=<html|console>                 - Format to output comparison ….<br class="">   Is there a standard way to do this?<br class=""><br class=""></blockquote>LLVM’s CommandLine library renders the help text. Also, I noticed I<br class="">used ‘comparison’ instead of ‘coverage’. Thanks!<br class=""><br class=""><blockquote type="cite" class="">3) Should ‘console’ be changed to ‘text’, because it doesn’t show in<br class="">the console when you give an output directory?<br class=""><br class=""></blockquote>Yeah, I think it could. I’ve cc’d Justin for his input — he originally<br class="">suggested Console.<br class=""></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">My only problem with "text" is that this includes ANSI escape sequences</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">when it uses colour, so text seems a bit misleading. That said, it</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">doesn't matter much - I'm fine with either.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><blockquote type="cite" class="">4) Could you please use an html charset that supports UTF-8?<br class=""><br class=""></blockquote>Yep. Thanks for catching that!<br class=""><br class=""><blockquote type="cite" class="">5) The HTML report can look quite different to the source file if it<br class="">includes tabs. Could you specify a smaller tab-size in the CSS file?<br class=""><br class=""></blockquote>That’s a good question, and I don’t quite know the correct answer to<br class="">it. GitHub has a complicated set of rules for tab sizing that makes a<br class="">‘best guess’ based on the source file type. I wonder if it’s a good<br class="">candidate for a command line switch?<br class=""><br class=""><blockquote type="cite" class="">In the future (not for this patch):<span class=""> </span><br class=""><br class="">6) Could we support a user CSS file?<span class=""> </span><br class="">Currently, the CSS rules are embedded into the html page. Could<br class="">llvm-cov provide an option to use an external CSS file? This would<br class="">give the user more flexibility.<br class=""><br class=""></blockquote>I agree that it’d give the user more flexibility, but it’d be<br class="">difficult to do given the nature of the directory structure. Maybe, if<br class="">you know you’re gonna be running it on a server where the ‘root’ is<br class="">well-defined, we could just dump the css file to disk and put a<br class="">relative <link> tag in. But if it’s gonna be ten directories down,<br class="">we’ll need to have some logic in that replaces all the parent paths in<br class="">the link with /../..’s to make the file resolve. I think it can be<br class="">done, just with more input from the user about their intention with<br class="">hosting.<br class=""><br class=""><blockquote type="cite" class="">7) Could we put the functions/lines summary information into the<br class="">HTML files? As a rough example:<br class=""><br class="">Lines: 9/10 (90%)<br class="">Functions: 1/1 (100%)<br class=""><br class=""></blockquote>I think this totally can be done — it’s out of scope for this patch<br class="">(it’s already a pretty big patch as it is.)<br class="">Some kind of merging of ‘show’ and ‘report’ would be ideal. Maybe the<br class="">index page has at-a-glance information about each file?<br class=""></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">Yep, this sounds like a great idea, but it's probably more suited to a</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">follow up patch.</span></div></blockquote></div><br class=""></div></div></div><br class=""></blockquote></div><br class=""><br clear="all" class=""><br class="">--<span class=""> </span><br class=""><div class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><font size="2" class=""><span style="font-family:arial,helvetica,sans-serif" class="">Ying Yi</span><span style="font-family:arial,helvetica,sans-serif" class=""><br class="">SN Systems Ltd - Sony Computer Entertainment Group.</span></font></div></div></div></div></div></div></div></blockquote></div><br class=""></div><br class=""></blockquote></div><br class=""><br clear="all" class=""><br class="">-- <br class=""><div class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><font size="2" class=""><span style="font-family:arial,helvetica,sans-serif" class="">Ying Yi</span><span style="font-family:arial,helvetica,sans-serif" class=""><br class="">SN Systems Ltd - Sony Computer Entertainment Group.</span><span style="font-family:arial,helvetica,sans-serif" class=""></span></font><br class=""></div></div></div></div></div></div>
</div></div>
</div></blockquote></div><br class=""></div></div><br class=""></blockquote></div><br class=""><br clear="all" class=""><br class="">-- <br class=""><div class="gmail_signature"><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><font size="2" class=""><span style="font-family:arial,helvetica,sans-serif" class="">Ying Yi</span><span style="font-family:arial,helvetica,sans-serif" class=""><br class="">SN Systems Ltd - Sony Computer Entertainment Group.</span><span style="font-family:arial,helvetica,sans-serif" class=""></span></font><br class=""></div></div></div></div></div></div>
</div>
</div></blockquote></div><br class=""></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></div></body></html>