[clang] Use `llvm::SmallVector` instead of `OwningArrayRef` in `VTableLayout`. (PR #168768)

[David Stone via cfe-commits]

================
@@ -2306,18 +2306,17 @@ MakeAddressPointIndices(const VTableLayout::AddressPointsMapTy &addressPoints,
   return indexMap;
 }
 
-VTableLayout::VTableLayout(ArrayRef<size_t> VTableIndices,
+VTableLayout::VTableLayout(VTableIndicesTy VTableIndices,
                            ArrayRef<VTableComponent> VTableComponents,
                            ArrayRef<VTableThunkTy> VTableThunks,
                            const AddressPointsMapTy &AddressPoints)
-    : VTableComponents(VTableComponents), VTableThunks(VTableThunks),
-      AddressPoints(AddressPoints), AddressPointIndices(MakeAddressPointIndices(
-                                        AddressPoints, VTableIndices.size())) {
-  if (VTableIndices.size() <= 1)
-    assert(VTableIndices.size() == 1 && VTableIndices[0] == 0);
-  else
-    this->VTableIndices = OwningArrayRef<size_t>(VTableIndices);
-
+    : VTableIndices(std::move(VTableIndices)),
+      VTableComponents(VTableComponents), VTableThunks(VTableThunks),
+      AddressPoints(AddressPoints),
+      AddressPointIndices(
+          MakeAddressPointIndices(AddressPoints, this->VTableIndices.size())) {
+  assert(!this->VTableIndices.empty() &&
+         "VTableLayout requires at least one index.");
----------------
davidstone wrote:

Good question. My understanding is that the old code asserted the contents because it was representing the set of indexes `{0}` as an empty set, so if that weren't the case we would theoretically have wrong data. Now that we just store the actual data passed in, that potential discrepancy is impossible. We still assert `!empty()` because other parts of the code assume you can at least index into the first element of the array so we would have undefined behavior if the size is not at least 1.

That being said, it is true that the first VTable index is 0, and you can prove that by looking at the code just in this file: the first index either comes from a manually defined set of `{0}` (line 3732) or from `ItaniumVTableBuilder::VTableIndices` (line 2417), which has `Components.size()` as its first element (line 1695), and `Components` is guaranteed to be empty the first time `LayoutPrimaryAndSecondaryVTables` is called. However, none of the code actually depends on this truth as far as I can tell (even though it would be really weird to have the first element at a different index!), and the original code didn't assert that the first index is always `0` for other sizes despite that being an identity that seems equally important regardless of the size of `VTableIndices`.

So we have three options here, as I see it:
1. Add the assertion back in for just the `size() == 1` case (to match the previous assertion)
2. Add an assert that the first index is always `0`, regardless of size (there isn't anything special about `size() == 1` anymore so it's kind of weird to assert only in that case)
3. Leave out the assertion since nothing appears to depend on it being true even though it is true

I don't have super strong feelings about any of these options.

https://github.com/llvm/llvm-project/pull/168768