[llvm-commits] [PATCH] Refactoring MemoryBuiltin Analysis

Chandler Carruth chandlerc at google.com
Mon Jun 18 17:06:28 PDT 2012


A couple of style nit picks from the peanut gallery...

On Mon, Jun 18, 2012 at 4:57 PM, Nick Lewycky <nlewycky at google.com> wrote:

> +/// isAllocLikeFn - returns true if V is a call to a malloc, calloc, or
> strdup
> +/// like function.
> +bool isAllocLikeFn(const Value *V, bool LookThroughBitCast = false);
>
> You've got to include some more detail in the doxystrings here to
> differentiate between isAllocationFn, isAllocLikeFn and isMallocLikeFn!
>

Also, I would prefer the more formal doxygen syntax, which removes the
redundant function name:

/// \brief Tests if a value is a call to a malloc, calloc, or strdup like
function.
/// ...
bool isAllocLikeFn(...)


> +// FIXME: can use unions here to save space
> +struct CacheData {
> +  APInt Offset;
> +  Value *OffsetValue;
> +  APInt Size;
> +  Value *SizeValue;
> +  bool ReturnVal;
> +  CacheData() {}
> +  CacheData(APInt Off, Value *OffVal, APInt Sz, Value *SzVal, bool Ret) :
> +    Offset(Off), OffsetValue(OffVal), Size(Sz), SizeValue(SzVal),
> +    ReturnVal(Ret) {}
> +};
>
> 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?
>
> Also, put the ":" of the initialize list on the same line as the first
> variable being initialized.
>

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.


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

Heh, I see Nick beat me to this, I just didn't get to it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120618/737dc0c1/attachment.html>


More information about the llvm-commits mailing list