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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 5 inline comments as done.
ldionne added inline comments.


================
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:
> 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.
In the next revision, I am actually removing `__builtin_bit_cast` entirely so this is moot.


================
Comment at: libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp:9
+
+// UNSUPPORTED: c++03
+// ADDITIONAL_COMPILE_FLAGS: -Wno-private-header
----------------
ldionne wrote:
> philnik wrote:
> > Why is this unsupported in C++03?
> Actually, for the same reason that `__bit_cast` won't work:
> 
> ```
> In file included from /c.strings/constexpr_memmove.pass.cpp:25:
> include/c++/v1/__string/constexpr_c_functions.h:211:66: error: copying parameter of type 'CopyConstructible' invokes deleted constructor
>         std::__assign_trivially_copyable(__dest + (__count - 1), __builtin_bit_cast(_Tp, __src[__count - 1]));
>                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /c.strings/constexpr_memmove.pass.cpp:92: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));
>                       ^
> /c.strings/constexpr_memmove.pass.cpp:112:3: note: in instantiation of function template specialization 'test_user_defined_types<CopyConstructible, CopyConstructible>' requested here
>   test_user_defined_types<CopyConstructible, CopyConstructible>();
>   ^
> /c.strings/constexpr_memmove.pass.cpp:41:3: note: 'CopyConstructible' has been explicitly marked deleted here
>   CopyConstructible(CopyConstructible&&)                 = delete;
>   ^
> ```
In the next update, I'm dropping `__builtin_bit_cast` and this test now works in C++03.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154613