[cfe-dev] !

[John McCall]

On Feb 15, 2012, at 4:37 AM, r4start wrote:
> Please see this patch.

You don't need the store both the set of vbases with vtordisps and the
additional information in the virtual-base-offset map.  I would prefer that
you rely only on the second;  it has the distinct virtual of imposing no
extra overload to the layouts of classes without virtual bases.

> I am not sure that new algorithm very efficient, but I think new algorithm is better than old one.

Why is this influenced by whether the class has a new virtual function?

Here's my suggested (untested) implementation:

void RecordLayoutBuilder::getRequiredVtorDisps(const CXXRecordDecl *RD, llvm::SmallPtrSet<const CXXRecordDecl*> &VBasesRequiringVtorDisp) {
  if (RD->vbases_empty()) return;

  // As an optimization, remember the set of virtual bases that we haven't decided yet.
  llvm::SmallPtrSet<const CXXRecordDecl *> UndecidedVBases;
  for (CXXRecordDecl::base_class_const_iterator I = RD->vbases_begin(),
       E = RD->vbases_end(); I != E; ++I) {
    UndecidedVBases.insert(*I);
  }

  // A virtual base requires a vtor disp if any of our base classes require it to have one.
  for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
       E = RD->bases_end(); I != E; ++I) {
    const CXXRecordDecl *BaseDecl = I->getType()->getAsCXXRecordDecl();
    const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
    for (ASTRecordLayout::VBaseOffsetsMapTy::const_iterator
          VI = BaseLayout.VBaseOffsetsMap.begin(), VE = BaseLayout.VBaseOffsetsMap.end(); VI != VE; ++VI) {
      if (!VI->second.IsVtorDispPresent) continue;
      if (!UndecidedVBases.erase(VI->first)) continue;
      VBasesRequiringVtorDisp.insert(VI->first);
      if (UndecidedVBases.empty()) return;
    }
  }

  // Otherwise, a virtual base requires a vtor disp if any the non-destructor
  // virtual methods declared in this class directly override a method provided
  // by that virtual base.

  // Collect the set of bases directly overridden by any method in this class.
  // It's possible that some of these classes won't be virtual bases, or won't be
  // provided by virtual bases, or won't be virtual bases in the overridden
  // instance but are virtual bases elsewhere.  Only the last matters for what
  // we're doing, and we can ignore those:  if we don't directly override
  // a method provided by a virtual copy of a base class, but we do directly
  // override a method provided by a non-virtual copy of that base class,
  // then we must indirectly override the method provided by the virtual base,
  // and so we should already have collected it in the loop above.
  llvm::SmallPtrSet<const CXXRecordDecl*> OverriddenBases;
  for (CXXRecordDecl::method_iterator M = RD->method_begin(),
       E = RD->method_end(); M != E; ++M) {
    if (!M->isVirtual() || isa<CXXDestructorDecl>(M)) continue;
    for (CXXMethodDecl::method_iterator I = M->begin_overridden_methods(),
          E = M->end_overridden_methods(); I != E; ++I) {
      const CXXRecordDecl *OverriddenBase = (*I)->getParent();

      // As an optimization, check immediately whether we're overriding
      // something from the undecided set.
      if (UndecidedVBases.erase(OverriddenBase)) {
        VBasesRequiringVtorDisp.insert(OverriddenBase);
        if (UndecidedVBases.empty()) return;
        continue;
      }

      // Otherwise, just collect it.
      OverriddenBases.insert(OverriddenBase);
    }
  }

  // Walk the undecided v-bases and check whether they (non-virtually)
  // provide any of the overridden bases.
  for (llvm::SmallPtrSet<const CXXRecordDecl*>::const_iterator
        I = UndecidedVBases.begin(), E = UndecidedVBases.end(); I != E; ++I) {
    if (hasAnyNonVirtualBase(*I, OverriddenBases))
      VBasesRequiringVtorDisp.insert(*I);
  }
}

Where hasAnyNonVirtualBase just recursively walks the non-virtual
class hierarchy and checks whether any of the bases are in the set.

> Also I refactor condition in 1472 line RecordLayoutBuilder.cpp.

Yes, that part looks fine;  you can separate that out as a different patch, I think.

John.