[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

A couple minor suggestions, then LGTM.



================
Comment at: clang/include/clang/AST/VTableBuilder.h:255
+    operator ArrayRef<T>() const { return {data(), size()}; };
+  };
+
----------------
Maybe this ought to be in LLVM as OwnedArrayRef?  And the more minimal implementation approach would be to inherit from MutableArrayRef<T> and just add a destructor and a move constructor.

The implicit conversion to ArrayRef is dangerous, but only in ways that ArrayRef is already dangerous.


================
Comment at: clang/include/clang/AST/VTableBuilder.h:260
+  // the vtable group contains a single vtable, an empty vector here represents
+  // the vector {0}.
+  FixedSizeVector<size_t> VTableIndices;
----------------
Ah.  Yes, that's a reasonable solution.


================
Comment at: clang/lib/AST/VTableBuilder.cpp:986
+  /// vtable group.
+  SmallVector<size_t, 1> VTableIndices;
+
----------------
Same comment here.  Might as well give this enough capacity for any reasonably foreseeable case.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:641
+llvm::Type *CodeGenVTables::getVTableType(const VTableLayout &layout) {
+  SmallVector<llvm::Type *, 1> tys;
+  for (unsigned i = 0, e = layout.getNumVTables(); i != e; ++i) {
----------------
While 1 is definitely the most likely size, adding small amounts of extra inline capacity to a temporary SmallVector has basically zero cost, and of course those cases do happen.  4 sounds reasonable.


https://reviews.llvm.org/D22296