[PATCH][RFC] llvm-cov HTML generation
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 3 13:03:31 PST 2016
This is great!
A few minor comments --
+ /// \brief Return a pointer to a stream for the given file.
+ /// Passing "" yields stdout.
+ std::unique_ptr<raw_ostream> streamForFile(std::string Filename);
I think llvm has autobrief turned on for doxygen, so the \brief isn't
strictly required.
Also, by convention function names are verb-y, so I suggest prefixing
this with a 'get' (http://llvm.org/docs/CodingStandards.html).
+void SourceCoverageViewHTML::output(raw_ostream &OS, std::string Text,
+ Optional<raw_ostream::Colors> Highlight,
+ const coverage::CoverageSegment
+ *Segment) {
+ auto Color = Highlight ? *Highlight : raw_ostream::SAVEDCOLOR;
+ Text = html::escape(Text);
I think the convention is to not modify parameters passed in by value.
Would it work for you to pass in std::string& Text and construct a new
std::string EscapedText?
+ auto Text = Line.substr(Col - 1, End - Col);
+ output(OS, Text, Highlight, S);
if (Options.Debug && Highlight)
HighlightedRanges.push_back(std::make_pair(Col, End));
Unrelated to this commit:
Maybe we should have a follow-up commit to switch to emplace_back(Col, End).
+void SourceCoverageViewHTML::renderSubviews(raw_ostream &OS,
+ line_iterator &LI,
+ std::vector<ExpansionView>::iterator &NextESV,
+ std::vector<ExpansionView>::iterator &EndESV,
+ std::vector<InstantiationView>::iterator &NextISV,
+ std::vector<InstantiationView>::iterator &EndISV,
+ const coverage::CoverageSegment *WrappedSegment,
+ SmallVector<const coverage::CoverageSegment *, 8>
+ LineSegments, unsigned IndentLevel,
+ unsigned ExpansionColumn, unsigned CombinedColumnWidth) {
+ for (; NextESV != EndESV && NextESV->getLine() == LI.line_number();
+ ++NextESV) {
Do you need the LineSegments parameter? If so, should it be passed by reference?
- for (; NextISV != EndISV && NextISV->Line == LI.line_number(); ++NextISV) {
This condition shows up enough to have a helper predicate for it. E.g
hasRemainingISVsOnLine(NextISV, EndISV, LI)
thanks
vedant
> On Mar 2, 2016, at 3:00 PM, Harlan Haskins via llvm-commits <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> wrote:
>>
>>
>>
>> On Wed, Mar 2, 2016 at 1:41 PM, Harlan Haskins <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> 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> 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> 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
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list