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

absl/container/inlined_vector.h

@@ -233,26 +233,15 @@ class InlinedVector {
     // remove its own reference to them. It's as if we had individually
     // remove its own reference to them. It's as if we had individually
     // move-constructed each value and then destroyed the original.
     // 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
     // TODO(b/274984172): a move construction followed by destroying the source
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // condition we need here (in addition to the allocator) is "trivially
     // condition we need here (in addition to the allocator) is "trivially
     // relocatable". Relaxing this would allow using memcpy with types like
     // relocatable". Relaxing this would allow using memcpy with types like
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // despite not being trivially move-constructible and/oror destructible.
     // 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 &&
         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_);
       storage_.MemcpyFrom(other.storage_);
       other.storage_.SetInlinedSize(0);
       other.storage_.SetInlinedSize(0);
       return;
       return;
@@ -298,26 +287,15 @@ class InlinedVector {
     // remove its own reference to them. It's as if we had individually
     // remove its own reference to them. It's as if we had individually
     // move-constructed each value and then destroyed the original.
     // 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
     // TODO(b/274984172): a move construction followed by destroying the source
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // is a "relocation" in the language of P1144R4. So actually the minimum
     // condition we need here (in addition to the allocator) is "trivially
     // condition we need here (in addition to the allocator) is "trivially
     // relocatable". Relaxing this would allow using memcpy with types like
     // relocatable". Relaxing this would allow using memcpy with types like
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // std::unique_ptr that opt in to declaring themselves trivially relocatable
     // despite not being trivially move-constructible and/oror destructible.
     // 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 &&
         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_);
       storage_.MemcpyFrom(other.storage_);
       other.storage_.SetInlinedSize(0);
       other.storage_.SetInlinedSize(0);
       return;
       return;

absl/container/internal/inlined_vector.h

@@ -510,16 +510,20 @@ class Storage {
     //
     //
     //  *  It's possible to trivially copy construct/assign the elements.
     //  *  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();
     GetSizeAndIsAllocated() = other_storage.GetSizeAndIsAllocated();
     data_ = other_storage.data_;
     data_ = other_storage.data_;