[PATCH] [ms-cxxabi] Implement vbtable name mangling

[Timur Iskhodzhanov]

  Overall looks good, a few questions below.


================
Comment at: include/clang/AST/Mangle.h:114
@@ +113,3 @@
+  virtual void mangleCXXVBTable(const CXXRecordDecl *Derived,
+                                llvm::ArrayRef<const CXXRecordDecl *> BasePath,
+                                raw_ostream &Out) = 0;
----------------
I'd prefer if you used `typedef <smth> CXXBaseSubpath` or `VBTablePath` (moved to AST/CXXInheritance.h?) instead.
Even though you only have 4 uses now, I'll have a few more uses when I finish my VFTable patch.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1741
@@ -1737,3 +1740,3 @@
                                              raw_ostream &Out) {
   // <mangled-name> ::= ? <operator-name> <class-name> <storage-class>
   //                      <cvr-qualifiers> [<name>] @
----------------
Copy this comment and replace `<operator-name>` with `_7`/`_8` in these two functions?
Ditto for `<storage-class>`.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1769
@@ +1768,3 @@
+  // Only a subset of the bases along the path to the vbtable are included in
+  // the name.  It's up to the caller to pick them correctly.
+  for (llvm::ArrayRef<const CXXRecordDecl *>::iterator I = BasePath.begin(),
----------------
Move this comment to `Mangler.h` ?

================
Comment at: lib/CodeGen/CGVTables.h:48
@@ +47,3 @@
+  /// The GlobalVariable for this vbtable.
+  llvm::GlobalVariable *GV;
+};
----------------
FYI, you won't be able to use GlobalVariable if we decide to move this code to AST.
How about storing a mangled name instead?

================
Comment at: lib/CodeGen/CGVTables.cpp:747
@@ +746,3 @@
+public:
+  VBTableBuilder(CodeGenModule &CGM, const CXXRecordDecl *MostDerived)
+    : CGM(CGM), MostDerived(MostDerived),
----------------
I think you can safely use Class or ForClass rather than MostDerived, since we don't have an intermediate "layout" class here as we need for VTableBuilder for Itanium ABI (due to construction tables and stuff).

================
Comment at: lib/CodeGen/CGVTables.cpp:805
@@ +804,3 @@
+                                          VBTablePathVector &Paths) {
+  size_t PathsStart = Paths.size();
+  bool ReuseChildVBPtr = true;
----------------
Should probably add a comment describing why you haven't used `CXXBasePaths`.

================
Comment at: lib/CodeGen/CGVTables.cpp:822
@@ +821,3 @@
+    CharUnits NextOffset;
+    const CXXRecordDecl *ChildForBase = Base;
+    if (I->isVirtual()) {
----------------
It's not immediately clear what ChildForBase is, so you might want to add a comment.

================
Comment at: lib/CodeGen/CGVTables.cpp:960
@@ +959,3 @@
+    CharUnits Offset = DerivedLayout.getVBaseClassOffset(VBase);
+    assert(Offset.getQuantity() >= 0);
+    // Make it relative to the subobject vbptr.
----------------
How about

  assert(!Offset.isNegative());

?

================
Comment at: test/CodeGenCXX/microsoft-abi-vbtables.cpp:96
@@ +95,3 @@
+namespace Test5 {
+// Test a non-virtual base with a virtual base that appears multiple times.
+struct A { int a; };
----------------
The comment is a bit confusing


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