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

[Ken Dyck]

On Friday, December 18, 2009 12:59 PM, Chris Lattner wrote:
> 
> 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?

That's the plan. Mainly in CodeGen code, but also touching Analysis,AST,
and a tiny bit of Sema.

A similar getTypeAlignInChars() is also in the plans.

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

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

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.

-Ken