rjmccall added a comment.
This seems reasonable to me. Just a few representational / API requests.
================
Comment at: clang/include/clang/AST/VTableBuilder.h:222
+ typedef llvm::DenseMap<BaseSubobject, std::pair<unsigned, unsigned>>
+ AddressPointsMapTy;
----------------
Please use a struct instead of std::pair.
================
Comment at: clang/include/clang/AST/VTableBuilder.h:225
private:
+ std::vector<size_t> VTableIndices;
+
----------------
Does this actually grow, or should you use a representation more like
VTableComponents? Or something custom that exploits the fact that, in the vast
majority of cases, this vector will have size 1?
================
Comment at: clang/include/clang/AST/VTableBuilder.h:243
+ uint64_t NumVTableThunks, const VTableThunkTy *VTableThunks,
+ const AddressPointsMapTy &AddressPoints, bool IsMicrosoftABI);
~VTableLayout();
----------------
Since you're changing this interface anyway, would you mind fixing it to use
ArrayRef for VTableComponents and VTableThunks, too?
================
Comment at: clang/include/clang/AST/VTableBuilder.h:260
+ assert(AddressPoint.second != 0 || IsMicrosoftABI);
(void)IsMicrosoftABI;
----------------
I know you didn't add this line, but could you remove this cast?
IsMicrosoftABI is not a local variable.
Also, the assert above suggests that this can be:
... = AddressPoint.find(Base)->second;
which should be slightly more efficient than lookup(). Also, I think the
assertion becomes unnecessary at that point.
================
Comment at: clang/lib/AST/VTableBuilder.cpp:986
+ /// vtable group.
+ std::vector<size_t> VTableIndices;
+
----------------
Assuming you stop storing a std::vector in the VTableLayout, you should make
this a SmallVector.
================
Comment at: clang/lib/CodeGen/CGVTables.cpp:644
+
+ std::vector<llvm::Type *> tys;
+ for (unsigned i = 0, e = idxs.size(); i != e; ++i) {
----------------
SmallVector, please.
================
Comment at: clang/lib/CodeGen/CGVTables.cpp:648
+ size_t nextIndex =
+ (i + 1 == e) ? layout.vtable_components().size() : idxs[i + 1];
+ tys.push_back(llvm::ArrayType::get(CGM.Int8PtrTy, nextIndex - thisIndex));
----------------
This seems like a completely reasonable query to add to VTableLayout:
unsigned getVTableSize(unsigned vtableIndex) const;
or maybe:
ArrayRef<VTableComponent> getVTableComponents(unsigned vtableIndex) const;
I guess you'll also need a getNumVTables().
https://reviews.llvm.org/D22296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
