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