[PATCH] Initial code coverage mapping data, reader, writer + C interface for ProfileData

[Alex L]

Thanks.
I was also wondering using a namespace for the coverage structures and
readers/writers - I feel like taking over the name Counter in the llvm's
namespace for coverage might not be such a good idea.


2014-07-18 17:13 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:

>
> On Jul 18, 2014, at 10:24 AM, Alex Lorenz <arphaman at gmail.com> wrote:
>
> Thanks again for the review, I've implemented the suggested changes. I
> also merged the coverage mapping data into one section - this reduced the
> object file size for the test programs by 4.5% on average.
>
> http://reviews.llvm.org/D4301
>
>
> Great! I was just hoping to simplify things by having fewer sections, so
> reducing the size overhead is a nice bonus.
>
> I have two other minor comments:
>
> 1) I noticed when reviewing your clang patch that you didn't consistently
> follow the coding standard with regard to function names starting with a
> lowercase letter, and I see some instances of that here, too. For example,
> CounterExpressionBuilder has some lowercase function (get, getExpressions)
> but also some uppercase ones (ExtractTerms, Simplify, Add and Subtract). By
> my reading of the coding convention, these should all start with lowercase
> letters. I think there are other instances of that. Please check.
>
> 2) I had suggested that you define a constant for Counter::EncodingTagBits
> + 1, which you did. But, you now have definitions of
> EncodingCounterTagAndExpansionRegionTagBits in both the reader and writer.
> I understand that this is only used internally, but it would be better to
> just define a single constant, probably alongside EncodingTagBits (i.e., in
> struct Counter).
>
> Please commit with those changes (or if you don’t have commit access yet,
> I can do it for you.)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140718/ec2e0401/attachment.html>