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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good, thanks!



================
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]);
 }
----------------
dexonsmith wrote:
> 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.
Looks good, don't have strong feelings about the last test case - take it or leave it as you see fit


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

https://reviews.llvm.org/D93779