[libcxx-commits] [PATCH] D124440: [libc++] Implement ranges::is_partitioned

[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/include/CMakeLists.txt:70
   __algorithm/ranges_copy.h
   __algorithm/ranges_copy_backward.h
   __algorithm/ranges_copy_if.h
----------------
Please also update the Ranges status page to mark this algorithm as done.


================
Comment at: libcxx/include/__algorithm/ranges_is_partitioned.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think you also need to update the synopsis in `<algorithm>`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
You also need to add the supported language versions/etc.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:9
+
+// <algorithm>
+
----------------
Please add synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:18
+template <class Iter, class Sent = sentinel_wrapper<Iter>>
+concept HasIsPartitionedIt = requires (Iter iter, Sent sent) {
+    std::ranges::is_partitioned(iter, sent, std::identity{});
----------------
Can you also check that this SFINAEs away if given something that isn't an `indirect_unary_predicate`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:19
+concept HasIsPartitionedIt = requires (Iter iter, Sent sent) {
+    std::ranges::is_partitioned(iter, sent, std::identity{});
+};
----------------
Ultranit: indent 2 spaces? Or is there a reason to use 4 spaces here?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:41
+
+template <class Iter, class Sent = Iter>
+constexpr void test_iterators() {
----------------
Optional: I think you could implement a helper function that takes care of both the iterator/sentinel and the ranges overloads. The main advantage would be more succinct callsites:
```
assert(test({1, 2, 3, 4, 5}, [](int i) { return i < 3; }));
assert(test({1, 2, 3, 4}, [](int i) { return true; }));
...
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:42
+template <class Iter, class Sent = Iter>
+constexpr void test_iterators() {
+  { // simple test
----------------
A few additional test cases:
- empty range;
- single-element range;
- the partition point is the first/the last element (check for off-by-one errors).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.partitions/ranges.is_partitioned.pass.cpp:56
+
+  { // check that it's partitoned if the predicate is true for all elements
+    {
----------------
Nit: `s/partitoned/partitioned/`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124440