[PATCH] D91837: ADT: Fix reference invalidation in SmallVector APIs that pass in a value

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

dexonsmith added a comment.

@njames93, thanks for the review, I've responded inline (I was out last week).



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:285-290
 template <typename T, bool = (is_trivially_copy_constructible<T>::value) &&
                              (is_trivially_move_constructible<T>::value) &&
                              std::is_trivially_destructible<T>::value>
 class SmallVectorTemplateBase : public SmallVectorTemplateCommon<T> {
 protected:
+  static constexpr bool TakesParamByValue = false;
----------------
Note: `TakesParamByValue` is always `false` when `T` is not copy-constructible.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:694
+    // update the pointer values in insert_one_impl.
+    return insert_one_impl(I, T(Elt));
+  }
----------------
njames93 wrote:
> Shouldn't this use the move constructor `T(std::move(Elt))` Otherwise this adds the unnecessary requirement that T is copyable when calling the xvalue version of insert.
The `enable_if` restricts this to when `TakesParamByValue`, which already requires that `T` is copyable (see note above). Do you think an expanded comment would be a good idea?

Or, maybe I've missed your point, let me know if I didn't understand. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91837