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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith marked 4 inline comments as done.
dexonsmith added a comment.

Thanks for the review! (Just coming back to this now after the holidays.)



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:711-713
+    static_assert(!TakesParamByValue || std::is_same<ArgType, T>::value,
+                  "ArgType must be 'T' when taking by value!");
+    if (!TakesParamByValue && this->isReferenceToRange(EltPtr, I, this->end()))
----------------
I added a static assert here to confirm we have a copy, since the logic relies on that.


================
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
----------------
dblaikie wrote:
> 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.
Good point; I've updated `...Impl()` to take/return `const`, and both callers now call it directly.


================
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;
----------------
dblaikie wrote:
> 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?
Correct. I added a comment and static assertion above to document this.


================
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]);
 }
----------------
dblaikie wrote:
> I'm not following what this part of the test does - could you walk me through it?
I've rewritten it (I couldn't fully make sense of it either, coming back to it). It now tests:
- The correct value gets copied when pushing back with a reference, growing from small storage.
- The correct value gets copied when pushing back with a reference, growing from large storage.
- The value is actually copied, not moved (the difference is only observable for Constructable). See `PushBackMoved` below, where a `0` is left behind. This seems potentially worth testing in case a `std::forward` is missed somewhere, but i'm open to dropping that part if you think it's already covered well enough above.


================
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());
----------------
dblaikie wrote:
> 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?
I've rewritten this in the same way as the `push_back` tests.


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

https://reviews.llvm.org/D93779