
<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><blockquote type="cite"><div>On Jul 18, 2014, at 5:34 PM, Alex L <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><div dir="ltr"><div>Thanks.<br></div>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.<br></div></div></blockquote><div><br></div>Sure, a separate namespace would make sense<div><br><div><blockquote type="cite"></blockquote></div></div><blockquote type="cite"><div><div dir="ltr">

</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-18 17:13 GMT-07:00 Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span>:<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"><div class=""><br><div><blockquote type="cite"><div>On Jul 18, 2014, at 10:24 AM, Alex Lorenz <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div>

<br><div>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.<br><br>

<a href="http://reviews.llvm.org/D4301" target="_blank">http://reviews.llvm.org/D4301</a></div></blockquote><br></div></div><div>Great! I was just hoping to simplify things by having fewer sections, so reducing the size overhead is a nice bonus.</div>

<div><br></div><div>I have two other minor comments:</div><div><br></div><div>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.</div>

<div><br></div><div>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).</div>

<div><br></div><div>Please commit with those changes (or if you don’t have commit access yet, I can do it for you.)</div></div></blockquote></div><br></div>

</div></blockquote></div><br></div></body></html>