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