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

[Chris Lattner]

On Dec 18, 2009, at 11:31 AM, Ken Dyck wrote:
>>> +++ 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.
>
> I don't think a forward declaration of the class is going to work  
> here.
> The getTypeSizeInChars() method returns a CharUnits, not a reference  
> or
> a pointer, so the full class definition is required.

A forward definition is enough, a caller just needs to include the  
header if it uses the method, but files that don't use the method  
don't need the definition of the class.

>> 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.
>
> It has shown up fairly frequently (5-15 times, roughly) in the
> prototyping that I've been doing on a private branch so far, so I'll
> look into adding something to CGM before I roll out any more of these.

Sounds great, thanks for working on this!

-Chris