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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
