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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

Thanks for working on this! It needs a bit more work.



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:31
+Read-only,is_sorted,Nikolas Klauser,` <https://llvm.org/>`_,✅
+Read-only,is_sorted_unitl,Nikolas Klauser,` <https://llvm.org/>`_,✅
 Read-only,includes,Not assigned,n/a,Not started
----------------
Please update to the right URL.


================
Comment at: libcxx/include/__algorithm/ranges_is_sorted.h:10
+#ifndef _LIBCPP__ALGORIUTHM_RANGES_IS_SORTED_H
+#define _LIBCPP__ALGORIUTHM_RANGES_IS_SORTED_H
+
----------------
3 occurrences here and the same for the guards of the `_until` header.


================
Comment at: libcxx/include/__algorithm/ranges_is_sorted.h:46
+                                          __comp,
+                                          __proj) == ranges::end(__range);
+  }
----------------
This looks sensible, however it seems the Standard doesn't specify this overload. Is there an LWG issue?


================
Comment at: libcxx/include/__algorithm/ranges_is_sorted_until.h:44
+  }
+  return std::move(__i);
+}
----------------
The move semantics in this function are inconsistent and wrong in some places.
`while (++__i != __last)` uses a moved from iterator.
one return moves, the other doesn't. Why move in the return, a `forward_iterator` is copyable.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
`libcxx/test/libcxx/algorithms/ranges_robust_against_copying_comparators.pass.cpp` and `libcxx/test/std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp` need to be updated.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:46
+
+  { // second element isn't sorted
+    {
----------------
Then please make the fifth element sorted.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:88
+
+  { // check that an empty range works
+    {
----------------
Can you also test a range with 1 element?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted.pass.cpp:111
+
+  { // check that the comparator is used
+    struct S {
----------------
Originally I wanted to say I missed tests for the projection ;-)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted_until.pass.cpp:47
+
+  { // second element isn't sorted
+    {
----------------
The comments for the sorted test apply here too, except this specific test.
You can update it, but since you test the location of the "failure" I deem the sorting of the last element not too important.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/is.sorted/ranges.is_sorted_until.pass.cpp:130
+  { // 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});
+  }
----------------
The interesting question is, does `std::ranges::sorted` work with a temporary?
I expect it should, but please add a test for sorted in the sorted test.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125608