[libcxx-commits] [PATCH] D125608: [libc++] Implement ranges::is_sorted{, _until}

[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.

Looks really good, just a few extra test cases to add plus a couple nits.



================
Comment at: libcxx/include/__algorithm/ranges_is_sorted.h:37
+  bool operator()(_Iter __first, _Sent __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    return ranges::__is_sorted_until_impl(__first, __last, __comp, __proj) == __last;
+  }
----------------
Nit: move?


================
Comment at: libcxx/include/__algorithm/ranges_is_sorted.h:46
+    return ranges::__is_sorted_until_impl(ranges::begin(__range),
+                                          ranges::end(__range),
+                                          __comp,
----------------
Nit: store the result in a variable so we don't have to call `end` twice?


================
Comment at: libcxx/include/__algorithm/ranges_is_sorted_until.h:54
+  _Iter operator()(_Iter __first, _Sent __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    return ranges::__is_sorted_until_impl(__first, __last, __comp, __proj);
+  }
----------------
Nit: `move` the iterator and the sentinel for consistency with other algorithms?


================
Comment at: libcxx/include/algorithm:270
+           indirect_strict_weak_order<projected<I, Proj>> Comp = ranges::less>
+    constexpr I ranges::is_sorted_until(I first, S last, Comp comp = {}, Proj proj = {});   // since C++20
+
----------------
A couple nits:
- in the standard, `is_sorted` goes before `is_sorted_until`, can you reorder for consistency?
- there's an empty line between two overloads of `is_sorted_until` but not `is_sorted`, can you make this consistent?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:34
+    {
+      int a[] = {1, 2, 3, 4, 3};
+      std::same_as<bool> auto ret = std::ranges::is_sorted(Iter(a), Sent(Iter(a + 5)));
----------------
Optional: consider creating a helper function that takes care of both the `(iterator, sentinel)` and the `(range)` overloads:
```
test_one({1, 2, 3, 4, 3}, /*expected=*/true); // Might need to use `std::array` as input
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:35
+      int a[] = {1, 2, 3, 4, 3};
+      std::same_as<bool> auto ret = std::ranges::is_sorted(Iter(a), Sent(Iter(a + 5)));
+      assert(!ret);
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:117
+
+constexpr bool test() {
+  test_iterators<forward_iterator<int*>>();
----------------
Please also add SFINAE tests.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:118
+constexpr bool test() {
+  test_iterators<forward_iterator<int*>>();
+  test_iterators<bidirectional_iterator<int*>>();
----------------
I think we also need to test the case when the sentinel is a different type from the iterator.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted_until.pass.cpp:30
+#include "test_iterators.h"
+
+template <class Iter, class Sent = Iter>
----------------
Same comments as the other test file.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted_until.pass.cpp:144
+  { // check that std::ranges::dangling is returned
+    [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret = std::ranges::is_sorted_until(std::array{1, 2, 3});
+  }
----------------
Nit: `decltype(auto)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125608