[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.
This revision is now accepted and ready to land.

LGTM with comments addressed and a green CI.



================
Comment at: libcxx/include/__algorithm/copy_backward.h:53
+              _BidirectionalIterator2 __result)
+{
+  return std::__copy_backward<_ClassicAlgPolicy>(
----------------
Please `static_assert` that the iterators are copyable.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:58
+  // At this point, the iterators have been unwrapped so any `contiguous_iterator` has been unwrapped to a pointer.
+  template <class _In, class _Out, __enable_if_t< is_trivially_assignable<_Out&, _In&>::value, int > = 0>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_In*, _Out*>
----------------
It's a weird quirk of clang-format right now and I don't know exactly why it does this, but these spaces can be removed and clang-format won't add them again. I think it's because of `SpacesInAngles: Keep`, but that shouldn't mean they are kept after a an opening one. Anyways, if you feel like removing these it would be nice, but not a must from my side.


================
Comment at: libcxx/include/__algorithm/move_backward.h:54
 {
-  return std::__move_backward<_ClassicAlgPolicy>(std::move(__first), std::move(__last), std::move(__result));
+  return std::__move_backward<_ClassicAlgPolicy>(
+      std::move(__first), std::move(__last), std::move(__result)).second;
----------------
Please also `static_assert` here.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:25
+
+template <size_t N, class Iter, std::enable_if_t<N == 0>* = nullptr>
+constexpr auto wrap_n_times(Iter i) {
----------------
Maybe use a `requires` since the test requires C++20 anyways?


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:106
+// that a call to `memmove` would fail to compile.
+template <>
+struct std::pointer_traits<::contiguous_iterator<NonTrivialCopyAssignment*>> {
----------------
This is technically still ill-formed, since the return type has to be `element_type*`, but I guess it's OK for us to do it in this case.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:23
+#include <type_traits>
+
+struct Foo {
----------------
To make it less prone to failing when we change includes, could you move the `<type_traits>` include down and `static_assert` the `trivially_copyable` later? It should still catch a problem before anything unpredictable happens. AFAICT you don't actually need the `<cstring>` include. `<cstddef>` should suffice for `size_t`.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:50
+
+template <size_t N, class Iter, std::enable_if_t<N == 0>* = nullptr>
+constexpr auto wrap_n_times(Iter i) {
----------------
Also `requires` here.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:68
+
+    Foo input[N] = {Foo{1}, {2}, {3}, {4}};
+    Foo output[N];
----------------
What is this `Foo` for? Shouldn't the compiler be able to deduce the correct type?


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_pointer_size.pass.cpp:9-11
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=2000000
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=2000000
+
----------------
Why is this required? Did it hit the constexpr limit without the `__builtin_memmove`? In that case, please remove the bumped limit again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695