[libcxx-commits] [PATCH] D122982: [libc++] Implement ranges::copy{, _n, _if, _backward}

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, several small issues.



================
Comment at: libcxx/include/__algorithm/copy.h:61
+                     && is_copy_constructible<_Sent>::value
+                     && is_copy_constructible<_OutIter>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Do we need to have an additional specialization? where
```
         __enable_if_t<is_copy_constructible<_InIter>::value
                      && is_copy_constructible<_Sent>::value
                      && !is_copy_constructible<_OutIter>::value> >
```
Just to make sure that a copy of a `std::string` to a non-copyable output iterator is still unwrapped.
This is will be used in `std::format`.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:56
-        __result -= __n;
-        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
-    }
----------------
Did you have a look at the codegen for this change?


================
Comment at: libcxx/include/__algorithm/ranges_copy.h:33
+template <class _Ip, class _Op>
+using copy_result = in_out_result<_Ip, _Op>;
+
----------------
`_InIter` and `_OutIter` for consistency?


================
Comment at: libcxx/include/__algorithm/ranges_copy_backward.h:31
+
+template<class _Ip, class _Op>
+using copy_backward_result = in_out_result<_Ip, _Op>;
----------------
:-) Happy you didn't follow the wording in the Standard.


================
Comment at: libcxx/include/__algorithm/ranges_copy_backward.h:37
+
+  template <bidirectional_iterator _I1, sentinel_for<_I1> _S1, bidirectional_iterator _I2>
+    requires indirectly_copyable<_I1, _I2>
----------------
I would suggest to use `_Ip` and `_Op` or change the `copy_backward_result` above.
In general this naming doesn't seem to use the new slightly longer names.


================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:49
+    }
+    return {std::move(__first), std::move(__result)};
+  }
----------------
The original is correct, but this is closer to the wording of the Standard.


================
Comment at: libcxx/include/algorithm:89
+    requires indirectly_copyable<I, O>
+    constexpr ranges::copy_result<I, O> ranges::copy(I first, S last, O result);            // since C++20
+
----------------
Are the `copy*result` types already in the synopsis?


================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/algorithm/ranges_copy.module.verify.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
These files will no longer be needed after D123028.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:160
+
+  { // check that the range is copied backwards
+    struct OnlyForwardsCopyable {
----------------
Seems a copy paste.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:13
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
+#include <algorithm>
----------------
Please add the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:112
+
+  { // check that the predicate is used
+    {
----------------
Please count the number of invocations the predicated, same for the projection below.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:150
+constexpr bool test() {
+  test_in_iterators<cpp17_output_iterator<int*>>();
+  test_in_iterators<forward_iterator<int*>>();
----------------
I would like to see tests for `cpp20_output_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982