[libcxx-commits] [PATCH] D125752: [libc++] Implement ranges::reverse

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

In general looks really good, thanks! (just a few comments)



================
Comment at: libcxx/include/__algorithm/ranges_reverse.h:40
+
+      while (__first < --__end) {
+        ranges::iter_swap(__first, __end);
----------------
Question: how is this loop more efficient than the non-random access iterator version?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse.pass.cpp:12
+
+// <algorithm>
+
----------------
Can you also check the complexity requirement?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse.pass.cpp:34
+static_assert(!HasReverseIt<BidirectionalIteratorNotDerivedFrom>);
+static_assert(!HasReverseIt<BidirectionalIteratorNotDecrementable>);
+
----------------
Can you also check the `permutable` constraint?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse.pass.cpp:47
+    auto val = value;
+    std::same_as<Iter> auto ret = std::ranges::reverse(Iter(val.data()), Sent(Iter(val.data() + val.size())));
+    assert(val == expected);
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse.pass.cpp:62
+constexpr void test_iterators() {
+  // simple test
+  test<Iter, Sent, 4>({1, 2, 3, 4}, {4, 3, 2, 1});
----------------
Please also check an odd number of elements.


================
Comment at: libcxx/test/support/almost_satisfies_types.h:142
 
+class BidirectionalIteratorNotDerivedFrom {
+public:
----------------
Nit: make it a `struct`, so that it won't be necessary to specify `public`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125752