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