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

[Reid Kleckner]

================
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;
----------------
Timur Iskhodzhanov wrote:
> 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.
The only AST header I'm touching is Mangle.h, so I want to put it there.  I'd also prefer to call it something generic like "ClassArrayRef".

================
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>] @
----------------
Timur Iskhodzhanov wrote:
> Copy this comment and replace `<operator-name>` with `_7`/`_8` in these two functions?
> Ditto for `<storage-class>`.
Done

================
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(),
----------------
Timur Iskhodzhanov wrote:
> Move this comment to `Mangler.h` ?
Done

================
Comment at: lib/CodeGen/CGVTables.h:48
@@ +47,3 @@
+  /// The GlobalVariable for this vbtable.
+  llvm::GlobalVariable *GV;
+};
----------------
Timur Iskhodzhanov wrote:
> 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?
I'm OK with that for now.  Currently this class is passed around by value, so I'm not keen to expand it with SmallString.

================
Comment at: lib/CodeGen/CGVTables.cpp:747
@@ +746,3 @@
+public:
+  VBTableBuilder(CodeGenModule &CGM, const CXXRecordDecl *MostDerived)
+    : CGM(CGM), MostDerived(MostDerived),
----------------
Timur Iskhodzhanov wrote:
> 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).
I'd rather be specific during the recursion.

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

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

Does that help clarify things?  I can't think of a good word for "the class whose virtual bases will fill the vbtable".  I'm trying to go with "the (potentially derived) class that is reusing this vbtable"

================
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.
----------------
Timur Iskhodzhanov wrote:
> How about
> 
>   assert(!Offset.isNegative());
> 
> ?
Done.


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