[libcxx-commits] [PATCH] D127211: [libc++] Implement ranges::{reverse, rotate}_copy

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:57
 Write,swap_ranges,Nikolas Klauser,`D116303 <https://llvm.org/D116303>`_,✅
-Write,reverse_copy,Not assigned,n/a,Not started
-Write,rotate_copy,Not assigned,n/a,Not started
+Write,reverse_copy,Nikolas Klauser,` <https://llvm.org/>`_,✅
+Write,rotate_copy,Nikolas Klauser,` <https://llvm.org/>`_,✅
----------------
As per usual, please update. :)


================
Comment at: libcxx/include/__algorithm/ranges_reverse_copy.h:19
+#include <__ranges/concepts.h>
+#include <__ranges/dangling.h>
+
----------------
Nit: include `<__utility/move.h>`.


================
Comment at: libcxx/include/__algorithm/ranges_reverse_copy.h:25
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
----------------
Please add the language version / no incomplete ranges guard (other file as well).


================
Comment at: libcxx/include/__algorithm/ranges_reverse_copy.h:39-40
+  reverse_copy_result<_InIter, _OutIter> operator()(_InIter __first, _Sent __last, _OutIter __result) const {
+    auto __ret = ranges::copy(std::__make_end_reverse_iterator(__first, __last),
+                              std::__make_begin_reverse_iterator(__first),
+                              std::move(__result));
----------------
These functions are verbose and using them seems a little clunky (they take a different number of arguments, it's not obvious whether e.g. `begin` in the name means "make `rbegin`" or "make `rend` from the begin iterator", etc.). How about having a single function that returns a pair of iterators instead? I don't think it should add any noticeable overhead:
```
auto [__rfirst, __rlast] = std::__to_reverse_iterators(__first, __last);
auto __ret = ranges::copy(std::move(__rfirst), std::move(__rlast), std::move(__result));
```


================
Comment at: libcxx/include/algorithm:1133
 #include <__algorithm/ranges_max.h>
 #include <__algorithm/ranges_max_element.h>
 #include <__algorithm/ranges_min.h>
----------------
Please add the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp:12
+
+// <algorithm>
+
----------------
Missing synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp:47
+    std::array<int, N> out;
+    std::same_as<std::ranges::in_out_result<Iter, OutIter>> auto ret =
+        std::ranges::reverse_copy(Iter(value.data()),
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp:12
+
+// <algorithm>
+
----------------
Missing synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp:23
+template <class Iter, class Out = int*, class Sent = sentinel_wrapper<Iter>>
+concept HasReverseCopyIt = requires(Iter first, Sent last, Out out) { std::ranges::rotate_copy(first, first, last, out); };
+
----------------
`s/Reverse/Rotate/`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp:47
+    std::array<int, N> out;
+    std::same_as<std::ranges::in_out_result<Iter, OutIter>> auto ret =
+        std::ranges::rotate_copy(Iter(value.data()),
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp:75
+
+  // check that a single element range works
+  test<Iter, OutIter, Sent, 1>({1}, 0, {1});
----------------
Please also check with a two-element range (the smallest range that can actually be rotated, and it has an even number of elements which otherwise is not tested).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127211