[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 added a comment.

In D93779#2497112 <https://reviews.llvm.org/D93779#2497112>, @dblaikie wrote:

> In D93779#2497099 <https://reviews.llvm.org/D93779#2497099>, @dexonsmith wrote:
>
>> Reverted in 56d1ffb927d03958a7a31442596df749264a7792 <https://reviews.llvm.org/rG56d1ffb927d03958a7a31442596df749264a7792> due to an error on Windows:
>> http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
>> if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.
>
> Guess maybe something in the `std::is_same<std::remove_const_t<std::remove_reference_t<ArgType>>, T>::value` isn't working quite right on MSVC? (could test that with godbolt, maybe) - is it needed? Since it's on both insert_one_maybe_copy and there's an implementation detail/doesn't need to protect from violations of that constraint (admittedly I'm proposing removing it because maybe MSVC is violating it... )? And instead SFINAE/enable_if only on the TakesParamByValue part?

Thanks for looking; I think you're right. The constraint was a trick to allow use of SFINAE since `TakesParamByValue` didn't depend on the template parameter so I can't just remove it, I need a different solution...

...but I've found one, and it's a bit cleaner too. Instead of the indirection through `insert_one_maybe_copy`, I've added a forwarding function defined in the POD vs. non-POD base classes called `forward_value_param`.

I'll commit the new version once I confirm `check-llvm` passes locally.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:370-371
+
+  static T &&forward_value_param(T &&V) { return std::move(V); }
+  static const T &forward_value_param(const T &V) { return V; }
+
----------------
In the non-POD case it just returns what it has been given.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:496-497
+
+  /// Copy \p V or return a reference, depending on \a ValueParamT.
+  static ValueParamT forward_value_param(ValueParamT V) { return V; }
+
----------------
In the POD (well, trivially move/copy constructible) case it's either `T` (a copy) or `const T&` (a reference). No need for moving since it's trivially constructible.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:727-733
   iterator insert(iterator I, T &&Elt) {
-    return insert_one_impl(I, std::move(Elt));
+    return insert_one_impl(I, this->forward_value_param(std::move(Elt)));
   }
 
-  iterator insert(iterator I, const T &Elt) { return insert_one_impl(I, Elt); }
+  iterator insert(iterator I, const T &Elt) {
+    return insert_one_impl(I, this->forward_value_param(Elt));
+  }
----------------
The new forwarding function also means we don't need to indirect through `instert_one_maybe_copy`.


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

https://reviews.llvm.org/D93779