[PATCH] llvm-cov: Added -a option for block data.

Justin Bogner mail at justinbogner.com
Mon Dec 9 16:30:22 PST 2013


LGTM.

Yuchen Wu <yuchenericwu at hotmail.com> writes:
> Here is the new patch, as per your suggestions. Comments below:
 ...
>>> diff --git a/include/llvm/Support/GCOV.h b/include/llvm/Support/GCOV.h
>>> index 3298132..c12b054 100644
>>> --- a/include/llvm/Support/GCOV.h
>>> +++ b/include/llvm/Support/GCOV.h
>>> @@ -37,6 +37,13 @@ namespace GCOV {
>>> };
>>> } // end GCOV namespace
>>>
>>> +/// GCOVOptions - A struct for passing gcov options between functions.
>>> +struct GCOVOptions {
>>> + GCOVOptions(bool A): AllBlocks(A) {}
>>
>> Is 'A' really better than 'AllBlocks'?
>
> As the number of options grow, it looks cleaner and shorter to have
> the options' shorthand as the variable names.

I disagree that it's cleaner, but I'll let you be the judge.

>>>
 ...
>>> AllLines = P.second;
>>> - ++i;
>>> +
>>> + // If -a option is enabled, output the counts for each block at the last
>>> + // line of the block.
>>> + if (Options.AllBlocks && BlocksIt != Line.end()) {
>>> + uint32_t BlockNo = 0;
>>> + const BlockVector &Blocks = BlocksIt->second;
>>> + for (BlockVector::const_iterator I = Blocks.begin(), E = Blocks.end();
>>> + I != E; ++I) {
>>
>> The whitespace is a bit funny here. The I should line up under the B in
>> BlockVector.
>
> Actually the style of this file (as I found it) seems to prefer adding
> an extra 2 whitespaces to multiline for statements.

That's a very strange style.



More information about the llvm-commits mailing list