[cfe-commits] [Review] rolling out ASTContext::getTypeSizeInBytes()

[Ted Kremenek]

I agree with John.  If we're rolling out a new API why not introduce now a new type that adds the "dimensionality" of the value?  (bytes versus bits)

John: Is that what you meant?

On Nov 24, 2009, at 1:25 PM, John McCall wrote:

> Ken Dyck wrote:
>> 
>> The attached patch rolls out the recently-added
>> ASTContext::getTypeSizeInBytes() method, replacing many (hopefully all)
>> of the expressions that calculate the byte size of a type by dividing
>> its bit size by a literal 8 or the bit size of the char type.
>> 
>> The changes are numerous but straighforward, touching portions of
>> CodeGen, Analysis, and AST. Could somebody review the patch before I
>> commit?
>>   
> 
> I'd feel a lot more confident in our correctness if we used an opaque type for all this. :)
> 
> 
> --- lib/AST/ExprConstant.cpp    (revision 89760)
> +++ lib/AST/ExprConstant.cpp    (working copy)
> @@ -424,7 +424,7 @@
>    if (PointeeType->isVoidType() || PointeeType->isFunctionType())
>      SizeOfPointee = 1;
>    else
> -    SizeOfPointee = Info.Ctx.getTypeSize(PointeeType) / 8;
> +    SizeOfPointee = Info.Ctx.getTypeSizeInBytes(PointeeType);
> 
> The void/function pointer case here also needs to be updated, yes?  Possibly to Info.Ctx.getCharWidth()?
> 
> Similarly here:
> 
> @@ -1156,7 +1156,7 @@
>  
>          uint64_t D = LHSValue.getLValueOffset() - RHSValue.getLValueOffset();
>          if (!ElementType->isVoidType() && !ElementType->isFunctionType())
> -          D /= Info.Ctx.getTypeSize(ElementType) / 8;
> +          D /= Info.Ctx.getTypeSizeInBytes(ElementType);
>  
>          return Success(D, E);
>        }
> 
> This implicitly assumes that arithmetic on void* is in 1-byte chunks.
> 
> --- lib/AST/ASTContext.cpp    (revision 89760)
> +++ lib/AST/ASTContext.cpp    (working copy)
> @@ -3085,15 +3085,15 @@
>  /// getObjCEncodingTypeSize returns size of type for objective-c encoding
>  /// purpose.
>  int ASTContext::getObjCEncodingTypeSize(QualType type) {
> -  uint64_t sz = getTypeSize(type);
> +  uint64_t sz = getTypeSizeInBytes(type);
>  
>    // Make all integer and enum types at least as large as an int
>    if (sz > 0 && type->isIntegralType())
> -    sz = std::max(sz, getTypeSize(IntTy));
> +    sz = std::max(sz, getTypeSizeInBytes(IntTy));
>    // Treat arrays as pointers, since that's how they're passed in.
>    else if (type->isArrayType())
> -    sz = getTypeSize(VoidPtrTy);
> -  return sz / getTypeSize(CharTy);
> +    sz = getTypeSizeInBytes(VoidPtrTy);
> +  return sz;
>  }
> 
> 
> You've changed this function from returning the size in chars to the size in bytes.  Intentional?  If so, please document. 
> 
> Everything else looks fine.
> 
> John.
> 
> 
>> Cheers,
>> -Ken
>>   
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>   
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20091124/19764ed3/attachment.html>