[cfe-commits] r65242 - in /cfe/trunk/lib/CodeGen: CGBlocks.cpp CGExprScalar.cpp CodeGenFunction.cpp CodeGenFunction.h

[Daniel Dunbar]

Some comments:

On Sat, Feb 21, 2009 at 12:00 PM, Mike Stump <mrs at apple.com> wrote:
> +  // FIXME: This breaks an unrelated testcase in the testsuite, we
> +  // _want_ llvm to not use structural equality, sometimes.  What
> +  // should we do, modify the testcase and do this anyway, or...
> +#if 0
>   getModule().addTypeName("struct.__block_descriptor",
>                           BlockDescriptorType);
> +#endif

Just fix the test case, the name assigned to equivalent types is
undefined. Would adding -fblocks=0 be a simple way to fix the test
case?

> +  // FIXME: second argument should be false?
>   llvm::Value *Func = Builder.CreateLoad(FuncPtr, FuncPtr, "tmp");

It certainly shouldn't be FunctPtr (for 'bool isVolatile' arg). false
is probably right, although I didn't look at context.

> +#include "llvm/Target/TargetData.h"

We shouldn't need this.

> +  const llvm::Type *Ty;
> +  Ty = CGF.CGM.getTypes().ConvertType(E->getDecl()->getType());
> +
> +  // See if we have already allocated an offset for this variable.
> +  if (offset == 0) {
> +    int Size = CGF.CGM.getTargetData().getTypeStoreSizeInBits(Ty) / 8;

Is there a good reason to be using TargetData here? Can't this use
ASTContext::getTypeSize() ?

> +    // Ensure proper alignment, even if it means we have to have a gap
> +    if (BlockOffset % (Align >> 3)) {
> +      BlockOffset += (Align >> 3) - (BlockOffset % (Align >> 3));
> +      assert ((BlockOffset % (Align >> 3)) == 0
> +              && "alignment calculation is wrong");
> +    }

Please use 'BlockOffset = RoundUpToAlignment(BlockOffset, Align/8);'

Also, wouldn't it be cleaner to  have getBlockOffset just take the
type and embed the size/align computation there?

 - Daniel