<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 8, 2016 at 10:25 AM, 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">Thanks again! Responses inline.<br><div><span class=""><blockquote type="cite"><div>On Mar 8, 2016, at 5:08 AM, 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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">Thanks for adding the index page. I have some comments:</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">1) The ‘</span><span style="font-size:10pt;background-image:none;background-color:white;background-position:0% 0%;background-repeat:repeat repeat">unistd.h</span><span style="font-size:10pt">’</span><span style="font-size:10pt"><span> </span>header won’t build on windows.</span></p><div><br></div></div></div></blockquote></span>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!<span class=""><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">2) The help text for ‘-format’ is different from llvm-profdata.exe tool, which uses the following style:</span></p><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"><span>   <span> </span></span>-format=<html|console><span>         <span> </span></span><span>       </span>- Format to output comparison ….</span></p><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"><span>   <span> </span></span>Is there a standard way to do this?</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div></div></div></blockquote></span>LLVM’s CommandLine library renders the help text. Also, I noticed I used ‘comparison’ instead of ‘coverage’. Thanks!<span class=""><br><blockquote type="cite"><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">3)<span> </span></span><span style="font-size:10pt">Should ‘console’ be changed to ‘text’, because it doesn’t show in the console when you give an output <span></span>directory?<span> </span></span></p><div><br></div></div></blockquote></span>Yeah, I think it could. I’ve cc’d Justin for his input — he originally suggested Console.<span class=""><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">4) Could you please use an<span> </span></span><span style="font-size:10pt">html charset that supports UTF-8?</span></p><div><br></div></div></div></blockquote></span>Yep. Thanks for catching that!<span class=""><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">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?</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div></div></div></blockquote></span><div>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?</div></div></div></blockquote><div><br></div><div><br></div><div>A command line switch is reasonable as a followup, I think.</div><div><br></div><div>David </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">In the future (not for this patch):<span> </span></span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">6) Could we support a user CSS file?<span> </span></span></p><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">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.</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"><span> </span></span><br></div></div></div></blockquote></span>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.</div><div><span class=""><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">7) Could we put the functions/lines summary information into the HTML files? As a rough example:<span> </span></span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"><span>          <span> </span></span></span><br></div><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">Lines: 9/10 (90%)</span></p><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">Functions: 1/1 (100%)</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div></div></div></blockquote></span><div>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.)</div><div>Some kind of merging of ‘show’ and ‘report’ would be ideal. Maybe the index page has at-a-glance information about each file?</div><div><div class="h5"><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" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">Many thanks,</span></p><div style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt"> </span><br></div><p class="MsoNormal" style="margin-bottom:0.0001pt;line-height:normal"><span style="font-size:10pt">Maggie</span></p></div></div></blockquote><br><blockquote type="cite"><div><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 Tue, Mar 8, 2016 at 12:56 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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">I really need to get better at patches.<div><br></div><div>Updated patch without the build directory:</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br><div><blockquote type="cite"><div>On Mar 7, 2016, at 2:03 PM, 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">Thanks! I’m still working out how to properly tests the HTML generation (I need to build a compatible version of clang to make compatible profdata inputs.)<div><br></div><div>In the meantime, I think I’ve fixed the portability issues that you and Maggie suggested, as well as a preliminary index implementation and a better design.</div><div><br></div><div>Let me know what you think!</div><div><br></div><div>Thanks,</div><div>Harlan</div><div><br></div><div></div></div><span><llvm-cov-html.diff></span><div style="word-wrap:break-word"><div></div></div><span><macro-coverage.zip></span><div style="word-wrap:break-word"><div></div><div><br><div><blockquote type="cite"><div>On Mar 7, 2016, at 1:26 PM, Xinliang David Li <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>The patch looks pretty good now.</div><div><br></div><div>>    std::string OutputPath = OutputDirectory;</div><div>>    if (OutputPath != "") {</div><div>>      sys::fs::create_directories(OutputDirectory);</div><div>>      OutputPath += "/functions." + FileExt;</div><div><br></div><div>This is not portable. Try:</div><div><br></div><div><div> <span> </span>llvm::sys::path::append(...) method</div></div><div><br></div><div>There are other places with similar code.</div><div><br></div><div>And it seems test cases are still missing?</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 4, 2016 at 10:46 AM, Harlan Haskins via llvm-commits<span> </span><span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Somehow the CSS include file didn’t make it into that diff.<div></div></div><br><div style="word-wrap:break-word"><div></div><div>— Harlan</div><div><br><div><blockquote type="cite"><div>On Mar 3, 2016, at 5:02 PM, 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">Hi all,<div><br></div><div>Thanks again for the reviews!</div><div><br></div><div>I’ve restructured how I handle subviews and pulled out the common behavior. I also addressed the issue with highlighting and showing macro expansions (and found a possible bug in clang because of it).</div><div><br></div><div>Attached is a) a new patch, and b) an HTML file showing a single line multi-macro expansion.</div><div><br></div><div>Thanks,</div><div>Harlan Haskins</div><div><br></div><div></div></div><span><macro.c.html></span><div style="word-wrap:break-word"><div></div></div><span><llvm-cov-html.diff></span><div style="word-wrap:break-word"><div><br><div><blockquote type="cite"><div>On Mar 2, 2016, at 3:00 PM, 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">Oh, I see! Yeah, this seems like something I overlooked. The HTML view currently just shows two expansions, one after the other.<div><br></div><div>I’ll revise the subview rendering with this in mind. Thanks for the simple example case!</div><div><br></div><div>Best,</div><div>Harlan</div><div><br><div><blockquote type="cite"><div>On Mar 2, 2016, at 2:40 PM, Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</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"><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 2, 2016 at 1:41 PM, 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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Hi David,<div><br></div><div>Specifically with renderSubviews, in my refactor it seemed that their bodies were different enough (i.e. SourceCoverageViewConsole needs to track state between loop invocations to know whether or not to display a final view divider, and the common behavior is really just looping over the instantiation and expansion subviews. I can investigate converging them more, but I think it’s going to increase complexity.</div></div></blockquote><div><br></div><div>Emitting the final divider is easy to abstract away. The main difference I see is that in Console view, if there are multiple macro expansions in the same line, the line will be re-rendered again in order to highlight the macro. For instance given the following line with two macros,</div><div>  </div><div>   MY_MACRO(10, 10); MY_MACRO(20,10);    // line 10</div><div><br></div><div>The console dump will be two lines:</div><div><br></div><div><font face="monospace, monospace">1|   10| MY_MACRO(10, 10); MY_MACRO(20,10);    // line 10</font></div><div><font face="monospace, monospace">         ^^^^^^^^^^^</font></div><div><font face="monospace, monospace">     <span> </span>.... expansion lines </font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">         MY_MACRO(10, 10); MY_MACRO(20,10);    // line 10</font><span style="font-family:monospace">                                                                     ^^^^^^^^^^^^^</span></div><div><font face="monospace, monospace">       <span> </span>.... expansion lines</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace"><br></font></div><div>Does HTML view lose that functionality? </div><div><br></div><div>Also the template instantiation subview rendering code looks sufficiently close between two classes.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>Also, I definitely need to add a test case.</div><div><br></div><div>Thanks!</div><span><font color="#888888"><div>Harlan</div></font></span><div><div><div><br><div><blockquote type="cite"><div>On Mar 2, 2016, at 10:43 AM, Xinliang David Li <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Hi Harlan,<div><br></div><div>This looks great! Some high level comments. I find the code can be further restructured</div><div>1) high level methods can be commoned between two derived classes  (and pushed to the base class) -- such as renderSubviews</div><div>2) the subclasses just need to provide virtual functions that implement the view specific low level routines.</div><div><br></div><div>Also there does not seem to be a test case.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 1, 2016 at 6:03 PM, Harlan Haskins via llvm-commits<span> </span><span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Oops, forgot to add a file to the patch.<div><br></div><div>New patch attached.</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br></div><div><br><div><blockquote type="cite"><div>On Mar 1, 2016, at 5:48 PM, 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">Hi all,<div><br></div><div>I’ve got a preliminary implementation of HTML generation for llvm-cov’s coverage reports.</div><div><br></div><div>The patch adds 2 flags to llvm-cov show:</div><div><ul><li>-format [html | console]</li><li>-output-dir <dirname></li></ul></div><div><br></div><div>Specifying -format=console will perform the current behavior (now the default), and -format=html will generate an HTML report.</div><div>If -output-dir is specified, then the output is split into one html or txt file per source file, named <source-name>.<ext>, with a directory structure that mimics the file system structure.</div><div><br></div><div>If neither are provided, the behavior remains the same.</div><div><br></div><div>I’m hoping to add an index with a browsable list of files within their directories, but for now I’ve attached the patch and a sample HTML file (In this case, AliasAnalysis.h, as included by swiftc).</div><div><br></div><div>Thanks,</div><div>Harlan Haskins</div><div><br></div><div></div></div><span><AliasAnalysis.h.html></span><div style="word-wrap:break-word"><div></div></div><span><llvm-cov-html.diff></span><div style="word-wrap:break-word"></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><br>_______________________________________________<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" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br><br></blockquote></div><br></div></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div></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">_______________________________________________</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">llvm-commits mailing list</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"><a href="mailto:llvm-commits@lists.llvm.org" 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">llvm-commits@lists.llvm.org</a><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"><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" 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">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></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>_______________________________________________<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><br>_______________________________________________<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" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br><br></blockquote></div><br></div></div></blockquote></div><br></div></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><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></div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>