[llvm-commits] [llvm] r166607 - in /llvm/trunk: include/llvm/DataLayout.h lib/Analysis/ConstantFolding.cpp lib/Analysis/InlineCost.cpp lib/Analysis/InstructionSimplify.cpp lib/CodeGen/AsmPrinter/AsmPrinter.cpp lib/ExecutionEngine/ExecutionEngine.

[Chandler Carruth]

On Wed, Oct 24, 2012 at 11:36 AM, Micah Villmow <villmow at gmail.com> wrote:
> Author: mvillmow
> Date: Wed Oct 24 13:36:13 2012
> New Revision: 166607
>
> URL: http://llvm.org/viewvc/llvm-project?rev=166607&view=rev
> Log:
> Add some cleanup to the DataLayout changes requested by Chandler.

And you've introduced quite a few really bad style violations. Please
review your patch for style before committing, or ask someone else
to...


> ==============================================================================
> --- llvm/trunk/include/llvm/DataLayout.h (original)
> +++ llvm/trunk/include/llvm/DataLayout.h Wed Oct 24 13:36:13 2012
> @@ -262,6 +262,14 @@
>      }
>      return 8*val->second.TypeBitWidth;
>    }

Vertical whitespace here.

> +  /// Layout pointer size, in bits, based on the type.
> +  /// If this function is called with a pointer type, then
> +  /// the type size of the pointer is returned.
> +  /// If this function is called with a vector of pointers,
> +  /// then the type size of the pointer is returned.
> +  /// Otherwise the type sizeo f a default pointer is returned.

If you're going to write a doxygen comment (and I ask that you do!) at
least use the doxygen structure. \brief and \returns would help this a
lot.

> +  unsigned getPointerTypeSizeInBits(Type* Ty)    const;

"Type *Ty" please, and only one space before the 'const'....

And then...

> -    unsigned AS = GV->getType()->getAddressSpace();
> -    unsigned PtrSize = TD->getPointerSizeInBits(AS)/8;
> +    assert(GV->getType()->isPointerTy() && "GV must be a pointer type!");
> +    unsigned PtrSize = TD->getTypeSizeInBits(GV->getType())/8;

Rather than an assert at each call site, why not just call
getPointerTypeSizeInBits(GV->getType()) and let that function assert?

> +      assert(CE->getOperand(1)->getType()->isPointerTy() &&
> +          "Must be a pointer type!");

And many of the asserts you've added are incorrectly indented such as this one.

> +unsigned DataLayout::getPointerTypeSizeInBits(Type *Ty) const
> +{

No, {s go on the previous line, always.

> +    if (Ty->isPointerTy()) return getTypeSizeInBits(Ty);
> +    if (Ty->isVectorTy()
> +        && cast<VectorType>(Ty)->getElementType()->isPointerTy())

Huh? cast<>() returns a pointer, always. It will assert if it is given
a null pointer or the type doesn't match.

> +      return getTypeSizeInBits(cast<VectorType>(Ty)->getElementType());
> +    return getPointerSizeInBits(0);

And this just doesn't make sense... Why do you allow non-pointer types
as arguments?

I think this function should be:

if (VectorType *VTy = dyn_cast<VectorType>(Ty))
  Ty = VTy->getElementType();
return getTypeSizeInBits(cast<PointerType>(Ty))