[PATCH] Fix vbtable indices if a class shares the vbptr with a non-virtual base

[Timur Iskhodzhanov]

================
Comment at: lib/AST/VTableBuilder.cpp:3314-3316
@@ +3313,5 @@
+    const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(CurBase);
+    if (!BaseLayout.hasVBPtr() ||
+        DerivedVBPtrOffset != BaseOffset + BaseLayout.getVBPtrOffset())
+      break;
+
----------------
Reid Kleckner wrote:
> I think this can just be:
> 
>   if (CurBase->getNumVBases() == 0)
>     continue;
> 
> No need to get the ASTRecordLayout of the nvbases.
Err, no - it leads to wrong getVBTableIndex() return values which result in later assertion failures in the VBTableBuilder.

After some thought, I've decided to bring back "continue;" rather than "break;".
This makes this code work correctly regardless of the order of nvbases returned by bases_begin()/end().
I don't think this orded is defined in such a way that the derived class is guaranteed to share its vbptr with first nvbase with vbases...
See the test I've added in the new iteration. It happens to work correctly now, but I'm not sure it's guaranteed to work with "break;" in the future.

I don't think we really want to optimize this way too much as the results are cached and there shouldn't be many classes with many nvbases.

================
Comment at: include/clang/AST/VTableBuilder.h:462
@@ +461,3 @@
+  typedef llvm::SmallSetVector<const CXXRecordDecl *, 8> BasesSetVectorTy;
+  void EnumerateVFPtrs(const CXXRecordDecl *MostDerivedClass,
+                       const ASTRecordLayout &MostDerivedClassLayout,
----------------
Reid Kleckner wrote:
> Start with lowercase 'e' to match the new conventions?
Good point, fixed!

================
Comment at: include/clang/AST/VTableBuilder.h:469
@@ +468,3 @@
+
+  void EnumerateVFPtrs(const CXXRecordDecl *ForClass,
+                       MicrosoftVTableContext::VFPtrListTy &Result);
----------------
Reid Kleckner wrote:
> ditto
ditto


http://llvm-reviews.chandlerc.com/D2089