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

[Mike Stump]

On Feb 21, 2009, at 1:32 PM, Daniel Dunbar wrote:
> Just fix the test case, the name assigned to equivalent types is
> undefined.

Ok, fixed.  I kinda don't like it.

> Would adding -fblocks=0 be a simple way to fix the test case?

Nope.

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

Fixed.

>> +#include "llvm/Target/TargetData.h"
>
> We shouldn't need this.

It was there before I got there, there is one more use of it, and that  
use seems to be based upon llvm types and ASTContext::getTypeSize()  
seems not to want one of those?

>> +  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() ?

I was copying code...  I fixed it.

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

Fixed.

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

Fixed.

Thanks.