[PATCH][RFC] llvm-cov HTML generation
Harlan Haskins via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 10:25:27 PST 2016
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.
> 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?
> Many thanks,
>
> Maggie
>
> On Tue, Mar 8, 2016 at 12:56 AM, Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> wrote:
> I really need to get better at patches.
>
> Updated patch without the build directory:
>
>
>
>> On Mar 7, 2016, at 2:03 PM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>> 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.)
>>
>> 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.
>>
>> Let me know what you think!
>>
>> Thanks,
>> Harlan
>>
>> <llvm-cov-html.diff>
>> <macro-coverage.zip>
>>
>>> On Mar 7, 2016, at 1:26 PM, Xinliang David Li <xinliangli at gmail.com <mailto:xinliangli at gmail.com>> wrote:
>>>
>>> The patch looks pretty good now.
>>>
>>> > std::string OutputPath = OutputDirectory;
>>> > if (OutputPath != "") {
>>> > sys::fs::create_directories(OutputDirectory);
>>> > OutputPath += "/functions." + FileExt;
>>>
>>> This is not portable. Try:
>>>
>>> llvm::sys::path::append(...) method
>>>
>>> There are other places with similar code.
>>>
>>> And it seems test cases are still missing?
>>>
>>> thanks,
>>>
>>> David
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Fri, Mar 4, 2016 at 10:46 AM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>> Somehow the CSS include file didn’t make it into that diff.
>>>
>>> — Harlan
>>>
>>>> On Mar 3, 2016, at 5:02 PM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Thanks again for the reviews!
>>>>
>>>> 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).
>>>>
>>>> Attached is a) a new patch, and b) an HTML file showing a single line multi-macro expansion.
>>>>
>>>> Thanks,
>>>> Harlan Haskins
>>>>
>>>> <macro.c.html>
>>>> <llvm-cov-html.diff>
>>>>
>>>>> On Mar 2, 2016, at 3:00 PM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>>
>>>>> Oh, I see! Yeah, this seems like something I overlooked. The HTML view currently just shows two expansions, one after the other.
>>>>>
>>>>> I’ll revise the subview rendering with this in mind. Thanks for the simple example case!
>>>>>
>>>>> Best,
>>>>> Harlan
>>>>>
>>>>>> On Mar 2, 2016, at 2:40 PM, Xinliang David Li via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 2, 2016 at 1:41 PM, Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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,
>>>>>>
>>>>>> MY_MACRO(10, 10); MY_MACRO(20,10); // line 10
>>>>>>
>>>>>> The console dump will be two lines:
>>>>>>
>>>>>> 1| 10| MY_MACRO(10, 10); MY_MACRO(20,10); // line 10
>>>>>> ^^^^^^^^^^^
>>>>>> .... expansion lines
>>>>>>
>>>>>> MY_MACRO(10, 10); MY_MACRO(20,10); // line 10 ^^^^^^^^^^^^^
>>>>>> .... expansion lines
>>>>>>
>>>>>>
>>>>>> Does HTML view lose that functionality?
>>>>>>
>>>>>> Also the template instantiation subview rendering code looks sufficiently close between two classes.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>
>>>>>>
>>>>>> Also, I definitely need to add a test case.
>>>>>>
>>>>>> Thanks!
>>>>>> Harlan
>>>>>>
>>>>>>> On Mar 2, 2016, at 10:43 AM, Xinliang David Li <xinliangli at gmail.com <mailto:xinliangli at gmail.com>> wrote:
>>>>>>>
>>>>>>> Hi Harlan,
>>>>>>>
>>>>>>> This looks great! Some high level comments. I find the code can be further restructured
>>>>>>> 1) high level methods can be commoned between two derived classes (and pushed to the base class) -- such as renderSubviews
>>>>>>> 2) the subclasses just need to provide virtual functions that implement the view specific low level routines.
>>>>>>>
>>>>>>> Also there does not seem to be a test case.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On Tue, Mar 1, 2016 at 6:03 PM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>>>> Oops, forgot to add a file to the patch.
>>>>>>>
>>>>>>> New patch attached.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On Mar 1, 2016, at 5:48 PM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I’ve got a preliminary implementation of HTML generation for llvm-cov’s coverage reports.
>>>>>>>>
>>>>>>>> The patch adds 2 flags to llvm-cov show:
>>>>>>>> -format [html | console]
>>>>>>>> -output-dir <dirname>
>>>>>>>>
>>>>>>>> Specifying -format=console will perform the current behavior (now the default), and -format=html will generate an HTML report.
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> If neither are provided, the behavior remains the same.
>>>>>>>>
>>>>>>>> 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).
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Harlan Haskins
>>>>>>>>
>>>>>>>> <AliasAnalysis.h.html>
>>>>>>>> <llvm-cov-html.diff>
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>
>
>
>
> --
> Ying Yi
> SN Systems Ltd - Sony Computer Entertainment Group.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160308/bd5c2f7b/attachment.html>
More information about the llvm-commits
mailing list