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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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>());
----------------
ldionne wrote:
> 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?
That's fine with me.


================
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 {
----------------
ldionne wrote:
> 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`).
Ugh. Ok, I'd be fine with using `__builtin_bit_cast` for now and adding a TODO to investigate further. There has to be some way to fix this.


================
Comment at: libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp:9
+
+// UNSUPPORTED: c++03
+// ADDITIONAL_COMPILE_FLAGS: -Wno-private-header
----------------
Why is this unsupported in C++03?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154613