[libcxx-commits] [PATCH] D154613: [libc++] Fix std::move algorithm with trivial move-only types

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D154613#4479865 <https://reviews.llvm.org/D154613#4479865>, @alexfh wrote:

> It looks like this fix will take some time to converge. Can we first return to green, please? The premerge checks succeeded on https://reviews.llvm.org/D154595

See https://reviews.llvm.org/D154595#4480304



================
Comment at: libcxx/include/__string/constexpr_c_functions.h:186-189
+          is_copy_assignable<_Tp>::value    ? 0 :
+          is_move_assignable<_Tp>::value    ? 1 :
+          is_copy_constructible<_Tp>::value ? 2 :
+          is_move_constructible<_Tp>::value ? 3 : -1>());
----------------
philnik wrote:
> Would it make sense to first try construction? Then we could use `__constexpr_memmove` for `uninitialized_{copy, move}` too.
I actually ran into issues doing this (I think it triggers some issues in older standard modes), plus the semantics of this function (as currently written) are to perform an assignment, so I think trying the assignment first makes sense. I'm definitely willing to investigate, but I'd like to do that in its own patch. Are you OK with that?


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:211
       for (; __count > 0; --__count)
-        __dest[__count - 1] = __src[__count - 1];
+        std::__assign_trivially_copyable(__dest + (__count - 1), __builtin_bit_cast(_Tp, __src[__count - 1]));
     } else {
----------------
philnik wrote:
> Why do you `bit_cast` here? Also, could we add a `__bit_cast` instead of using builtins randomly in the code?
I `bit_cast` in order to avoid having to deal with `_Tp` and `_Up` in the assignment -- I want to deal with a single type over there.

> Also, could we add a __bit_cast instead of using builtins randomly in the code?

Yes, I can do that.

Edit: I just tried introducing `__bit_cast` and using it, and it actually triggers some test failures in C++11 and C++14 mode:

```
In file included from libcxx/strings/c.strings/constexpr_memmove.pass.cpp:25:
In file included from include/c++/v1/__string/constexpr_c_functions.h:12:
include/c++/v1/__bit/bit_cast.h:36:10: error: call to deleted constructor of 'CopyConstructible'
  return __builtin_bit_cast(_ToType, __from);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/c++/v1/__string/constexpr_c_functions.h:212:71: note: in instantiation of function template specialization 'std::__bit_cast<CopyConstructible, CopyConstructible>' requested here
        std::__assign_trivially_copyable(__dest + (__count - 1), std::__bit_cast<_Tp>(__src[__count - 1]));
                                                                      ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:91:23: note: in instantiation of function template specialization 'std::__constexpr_memmove<CopyConstructible, CopyConstructible, 0>' requested here
  Dest* result = std::__constexpr_memmove(dst, src, std::__element_count(3));
                      ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:111:3: note: in instantiation of function template specialization 'test_user_defined_types<CopyConstructible, CopyConstructible>' requested here
  test_user_defined_types<CopyConstructible, CopyConstructible>();
  ^
libcxx/strings/c.strings/constexpr_memmove.pass.cpp:40:3: note: 'CopyConstructible' has been explicitly marked deleted here
  CopyConstructible(CopyConstructible&&)                 = delete;
  ^
```

I think this is because RVO isn't mandated before C++17 (IIRC?), and so we actually require a move constructor to return from `__bit_cast` for some of the funky types I use in the test. So it looks like I might need to keep using `__builtin_bit_cast` directly here (even though I agree with you it would make more sense to use some `std::__bit_cast`).


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp:40
     {
-    int a1[] = {1, 2, 3, 4, 5};
-    std::vector<int> l1(a1, a1+5);
-    l1.erase(l1.begin());
-    assert(is_contiguous_container_asan_correct(l1));
-    assert(l1 == std::vector<int>(a1+1, a1+5));
+        int a1[] = {1, 2, 3, 4, 5};
+        std::vector<int> l1(a1, a1+5);
----------------
philnik wrote:
> Instead of moving all the code an indentation deeper, maybe move the braces an indentation higher? That would basically change this test to use two-space indentation instead of four spaces, like we do in new code. (or just clang-format the whole thing)
I'll clang-format it, you're right the diff is actually smaller.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154613