As reported in https://reviews.llvm.org/D151953#4472195, the std::move
algorithm (and various other functions that relied on it) stopped working
after starting to use __constexpr_memmove in its implementation. This
patch fixes that and adds tests for various related algorithms and
functions that were not exercising trivial move-only types.
Details
- Reviewers
philnik - Group Reviewers
Restricted Project - Commits
- rG6f36ead577df: [libc++] Fix std::move algorithm with trivial move-only types
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
(This doesn't fix anything yet, this only adds the regression tests. I'm meeting with @philnik in a few minutes and we can discuss what we think is the best way to address this)
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
183–186 | Would it make sense to first try construction? Then we could use __constexpr_memmove for uninitialized_{copy, move} too. | |
206 | Why do you bit_cast here? Also, could we add a __bit_cast instead of using builtins randomly in the code? | |
libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp | ||
54–55 | 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) |
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
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
183–186 | 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? | |
206 | 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.
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). | |
libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp | ||
54–55 | I'll clang-format it, you're right the diff is actually smaller. |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
183–186 | That's fine with me. | |
206 | 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. | |
libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp | ||
10 | Why is this unsupported in C++03? |
libcxx/test/libcxx/strings/c.strings/constexpr_memmove.pass.cpp | ||
---|---|---|
10 | 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; ^ |
LGTM with a comment.
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
171 | Could you add a comment here explaining that we implicitly end the lifetime of the old object and then start the lifetime of the new object? I think this is a quite obscure part of the standard. | |
179 | Having if constexpr would make this so much nicer :( |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
171 |
From https://en.cppreference.com/w/cpp/language/lifetime (under storage reuse). Comment added x2. |
Could you add a comment here explaining that we implicitly end the lifetime of the old object and then start the lifetime of the new object? I think this is a quite obscure part of the standard.