[PATCH] D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:480-490
+  /// Reserve enough space to add one element, and return the updated element
+  /// pointer in case it was a reference to the storage.
+  T *reserveForAndGetAddress(T &Elt) {
+    return this->reserveForAndGetAddressImpl(this, Elt);
+  }
+
+  /// Reserve enough space to add one element, and return the updated element
----------------
While I think it's not technically a problem (because the casted-away-const-ness pointer is never dereferenced) - I'd usually implement these the other way around. Have an implementation that uses const pointers only, then the non-const wrapper can cast-to-const, call, cast away const, and it's "clearer" (marginally, perhaps) that it's const correct because only that non-const wrapper has the interesting const handling - rather than the longer distance behavior in this implementation.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:693-694
+    size_t Index = I - this->begin();
+    std::remove_reference_t<ArgType> *EltPtr =
+        this->reserveForAndGetAddress(Elt);
+    I = this->begin() + Index;
----------------
Is this correct? If ArgType differs from the container's value type, wouldn't this break? (if you insert something that's convertible to the element type, but isn't the actual element type)

I guess insert_one_maybe_copy does SFINAE, etc to ensure this is always called with the value type? So ArgType is only ever some kind of ref qualified T?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1088-1089
+  EXPECT_EQ(N, V.back());
+  if (this->template isValueType<Constructable>())
+    EXPECT_EQ(N, V[N - 1]);
 }
----------------
I'm not following what this part of the test does - could you walk me through it?


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1172-1176
+  // The logic for reference invalidation when NOT growing is disabled when
+  // TakesParamByValue since it relies on taking a copy. Check that we actually
+  // took a copy.
+  V.insert(V.begin(), V.back());
+  EXPECT_EQ(N, V.front());
----------------
I'm not following how this validates that a copy was taken (I'd have thought it'd be unobservable whether a copy was made or the pointer-index-pointer dance was done) - could you walk me through that in a bit more detail?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93779/new/

https://reviews.llvm.org/D93779