<div style="font-family: arial, helvetica, sans-serif"><font size="2">A couple of style nit picks from the peanut gallery...<div><br><div><div class="gmail_quote">On Mon, Jun 18, 2012 at 4:57 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-family:arial,helvetica,sans-serif"><font><div>+/// isAllocLikeFn - returns true if V is a call to a malloc, calloc, or strdup</div>
<div><div>+/// like function.</div><div>+bool isAllocLikeFn(const Value *V, bool LookThroughBitCast = false);</div>
</div>
<div><br></div><div>You've got to include some more detail in the doxystrings here to differentiate between isAllocationFn, isAllocLikeFn and isMallocLikeFn!</div></font></div></blockquote><div><br></div><div>Also, I would prefer the more formal doxygen syntax, which removes the redundant function name:</div>
<div><br></div><div>/// \brief Tests if a value is a call to a malloc, calloc, or strdup like function.</div><div>/// ...</div><div>bool isAllocLikeFn(...)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="font-family:arial,helvetica,sans-serif"><font><div>+// FIXME: can use unions here to save space</div><div>

<div>+struct CacheData {</div><div>+  APInt Offset;</div><div>+  Value *OffsetValue;</div><div>+  APInt Size;</div><div>+  Value *SizeValue;</div><div>+  bool ReturnVal;</div><div>+  CacheData() {}</div><div>+  CacheData(APInt Off, Value *OffVal, APInt Sz, Value *SzVal, bool Ret) :</div>


<div>+    Offset(Off), OffsetValue(OffVal), Size(Sz), SizeValue(SzVal),</div><div>+    ReturnVal(Ret) {}</div><div>+};</div></div><div><br></div><div>What are these? Each member should have a comment. Are "APInt <X>" and "Value *<X>Value" mutually exclusive? If so, why have a constructor that specifies all of them? Also, what does it really mean to construct the empty one, besides the fact that you can insert it into a DenseMap?</div>


<div><br></div><div>Also, put the ":" of the initialize list on the same line as the first variable being initialized.</div></font></div></blockquote><div><br></div><div>There is a nice convention of using the exact same constructor parameter names as member names that I think would also make this more readable. I think we're getting a bit carried away with creative abbreviations here, and C++ allows you to just re-use the member names in this context.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-family:arial,helvetica,sans-serif"><font><div>Please don't use leading underscores! That's reserved. I personally like to just use the exact same names as the members (yes, the scoping rules work out such that "TD(TD)" will initialize TD the member from TD the constructor argument) but if you don't like that place the underscores on the end.</div>
</font></div></blockquote><div><br></div><div>Heh, I see Nick beat me to this, I just didn't get to it.</div></div></div></div></font></div>