================
@@ -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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
