[cfe-commits] r76348 - in /cfe/trunk: lib/AST/RecordLayoutBuilder.cpp lib/AST/RecordLayoutBuilder.h test/SemaCXX/class-layout.cpp

[Anders Carlsson]

On Jul 20, 2009, at 3:57 PM, Douglas Gregor wrote:

>
> On Jul 18, 2009, at 5:18 PM, Anders Carlsson wrote:
>
>> Author: andersca
>> Date: Sat Jul 18 19:18:47 2009
>> New Revision: 76348
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=76348&view=rev
>> Log:
>> Handle layout of non-virtual base classes.
>
> Cool :)
>
>> +void ASTRecordLayoutBuilder::LayoutNonVirtualBase(const  
>> CXXRecordDecl *RD) {
>> +  const ASTRecordLayout &BaseInfo = Ctx.getASTRecordLayout(RD);
>> +    assert(BaseInfo.getDataSize() > 0 &&
>> +           "FIXME: Handle empty classes.");
>
> Yeah, that's a good way to start out. Empty base classes cause a  
> ruckus.

Yup :)

>
>> +  // FIXME: Should get the non-virtual alignment of the base.
>> +  unsigned BaseAlign = BaseInfo.getAlignment();
>> +
>> +  // FIXME: Should get the non-virtual size of the base.
>> +  uint64_t BaseSize = BaseInfo.getDataSize();
>> +
>> +  // Round up the current record size to the base's alignment  
>> boundary.
>> +  Size = (Size + (BaseAlign-1)) & ~(BaseAlign-1);
>> +
>> +  // Reserve space for this base.
>> +  Size += BaseSize;
>> +
>> +  // Remember the next available offset.
>> +  NextOffset = Size;
>> +
>> +  // Remember max struct/class alignment.
>> +  UpdateAlignment(BaseAlign);
>> +}
>
> Looks fine for now. Those three FIXME's imply a bunch of work that  
> will make this code a bit more complicated in the future.
>
> I think ASTRecordLayout store some information mapping direct base  
> classes to their offsets? We'll need that for derived-to-base and  
> base-to-derived cases.
>

Yeah, and for classes with a vtable we need to store the primary base  
class as well. Since this is only useful for C++ objects, we could add  
a new class that derives from ASTRecordLayout.

>> -  return new ASTRecordLayout(Builder.Size, Builder.Alignment,
>> -                             Builder.NextOffset,
>> +  bool IsPODForThePurposeOfLayout;
>> +  if (!Ctx.getLangOptions().CPlusPlus) {
>> +    // In C, all record types are POD.
>> +    IsPODForThePurposeOfLayout = true;
>> +  } else {
>> +    // FIXME: This is not always correct. See the part about  
>> bitfields at
>> +    // http://www.codesourcery.com/public/cxx-abi/abi.html#POD for  
>> more info.
>> +    IsPODForThePurposeOfLayout = cast<CXXRecordDecl>(D)->isPOD();
>> +  }
>
> Oh, yuck. I guess we'll have to add an  
> IsPODForThePurposeOfItaniumABILayout bit to ASTRecordLayout to track  
> this, because we certainly don't want to perform this (recursive)  
> computation for each record layout.
>

Yup. Could go into the same place as the rest of the C++ information.  
Thanks for the review!

Anders