inlined_vector: fix incorrect conditions for move constructor fast paths.

These have nothing to do with copy construction or copy assignment, and using
"is trivially copy constructible" can in theory even give the wrong result if
the copy constructor is trivial but the move constructor is not.

PiperOrigin-RevId: 520488816
Change-Id: I6da4d57f3ce23b03044e0bf9aa70a14b51651fa3

2 files changed, 18 insertions, 36 deletions

diff --git a/absl/container/inlined_vector.h b/absl/container/inlined_vector.h
index f5b819a1..6268eebf 100644
--- a/absl/container/inlined_vector.h
+++ b/absl/container/inlined_vector.h
@@ -233,26 +233,15 @@ class InlinedVector {
     // remove its own reference to them. It's as if we had individually
     // move-constructed each value and then destroyed the original.
     //
-    // TODO(b/274984172): we check for copy-constructibility here only for
-    // historical reasons. This is too strict: we are simulating move
-    // construction here. In fact this is arguably incorrect, since in theory a
-    // type might be trivially copy-constructible but not trivially
-    // move-constructible.
-    //
-    // TODO(b/274984172): the condition on copy-assignability is here only for
-    // historical reasons. It doesn't make semantic sense: we don't need to be
-    // able to copy assign here, we are doing an "as-if" move construction.
-    //
     // TODO(b/274984172): a move construction followed by destroying the source
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // condition we need here (in addition to the allocator) is "trivially
     // relocatable". Relaxing this would allow using memcpy with types like
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // despite not being trivially move-constructible and/oror destructible.
-    if (absl::is_trivially_copy_constructible<value_type>::value &&
+    if (absl::is_trivially_move_constructible<value_type>::value &&
         absl::is_trivially_destructible<value_type>::value &&
-        std::is_same<A, std::allocator<value_type>>::value &&
-        absl::is_trivially_copy_assignable<value_type>::value) {
+        std::is_same<A, std::allocator<value_type>>::value) {
       storage_.MemcpyFrom(other.storage_);
       other.storage_.SetInlinedSize(0);
       return;
@@ -298,26 +287,15 @@ class InlinedVector {
     // remove its own reference to them. It's as if we had individually
     // move-constructed each value and then destroyed the original.
     //
-    // TODO(b/274984172): we check for copy-constructibility here only for
-    // historical reasons. This is too strict: we are simulating move
-    // construction here. In fact this is arguably incorrect, since in theory a
-    // type might be trivially copy-constructible but not trivially
-    // move-constructible.
-    //
-    // TODO(b/274984172): the condition on copy-assignability is here only for
-    // historical reasons. It doesn't make semantic sense: we don't need to be
-    // able to copy assign here, we are doing an "as-if" move construction.
-    //
     // TODO(b/274984172): a move construction followed by destroying the source
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // condition we need here (in addition to the allocator) is "trivially
     // relocatable". Relaxing this would allow using memcpy with types like
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // despite not being trivially move-constructible and/oror destructible.
-    if (absl::is_trivially_copy_constructible<value_type>::value &&
+    if (absl::is_trivially_move_constructible<value_type>::value &&
         absl::is_trivially_destructible<value_type>::value &&
-        std::is_same<A, std::allocator<value_type>>::value &&
-        absl::is_trivially_copy_assignable<value_type>::value) {
+        std::is_same<A, std::allocator<value_type>>::value) {
       storage_.MemcpyFrom(other.storage_);
       other.storage_.SetInlinedSize(0);
       return;
diff --git a/absl/container/internal/inlined_vector.h b/absl/container/internal/inlined_vector.h
index 6397db48..4e628424 100644
--- a/absl/container/internal/inlined_vector.h
+++ b/absl/container/internal/inlined_vector.h
@@ -510,16 +510,20 @@ class Storage {
     //
     //  *  It's possible to trivially copy construct/assign the elements.
     //
-    // TODO(b/274984172): the conditions here, preserved from historical ones,
-    // don't actually implement this. They are far too conservative (they don't
-    // work for move-only types, and require both copyability and
-    // assignability).
-    ABSL_HARDENING_ASSERT(
-        other_storage.GetIsAllocated() ||
-        (std::is_same<A, std::allocator<ValueType<A>>>::value &&
-         absl::is_trivially_copy_constructible<ValueType<A>>::value &&
-         absl::is_trivially_copy_assignable<ValueType<A>>::value &&
-         absl::is_trivially_destructible<ValueType<A>>::value));
+    {
+      using V = ValueType<A>;
+      ABSL_HARDENING_ASSERT(
+          other_storage.GetIsAllocated() ||
+          (std::is_same<A, std::allocator<V>>::value &&
+           (
+               // First case above
+               ((absl::is_trivially_move_constructible<V>::value ||
+                 absl::is_trivially_move_assignable<V>::value) &&
+                absl::is_trivially_destructible<V>::value) ||
+               // Second case above
+               (absl::is_trivially_copy_constructible<V>::value ||
+                absl::is_trivially_copy_assignable<V>::value))));
+    }
 
     GetSizeAndIsAllocated() = other_storage.GetSizeAndIsAllocated();
     data_ = other_storage.data_;