[cfe-commits] r91689 - in /cfe/trunk: include/clang/AST/ASTContext.h lib/CodeGen/CGDecl.cpp

[Chris Lattner]

On Dec 18, 2009, at 7:55 AM, Ken Dyck wrote:

> Author: kjdyck
> Date: Fri Dec 18 09:55:54 2009
> New Revision: 91689
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=91689&view=rev
> Log:
> Change the return type of ASTContext::getTypeSizeInChars() from uint64_t to the
> new opaque value type, CharUnits. This will help us avoid accidentally mixing 
> quantities that are in bit and character units.

Nice.  I assume that this will start getting more pervasive throughout the codebase?

> +++ cfe/trunk/include/clang/AST/ASTContext.h Fri Dec 18 09:55:54 2009
> @@ -18,6 +18,7 @@
> #include "clang/Basic/LangOptions.h"
> #include "clang/Basic/OperatorKinds.h"
> #include "clang/AST/Attr.h"
> +#include "clang/AST/CharUnits.h"

Please move the bodies of getTypeSizeInChars out of line to avoid this #include.

> +++ cfe/trunk/lib/CodeGen/CGDecl.cpp Fri Dec 18 09:55:54 2009
> @@ -471,7 +471,8 @@
>       const llvm::Type *IntPtr =
>         llvm::IntegerType::get(VMContext, LLVMPointerWidth);
>       llvm::Value *SizeVal =
> -        llvm::ConstantInt::get(IntPtr, getContext().getTypeSizeInChars(Ty));
> +        llvm::ConstantInt::get(IntPtr, 
> +                               getContext().getTypeSizeInChars(Ty).getRaw());

Is this going to be a common operation?  If so, it might make sense to add a "getLLVMConstantInt(const llvm::Type*)" to CharUnit.  Actually, on second thought, that would be a layering violation because AST shouldn't depend on LLVM IR.  Maybe this should be a method on CGM?  Something like CGM.getSizeConstant(getContext().getTypeSizeInChars(Ty)) ?

I don't know if this will happen enough to make it worthwhile though.

-Chris