[libcxx-commits] [PATCH] D105794: [libcxx][algorithms] adds ranges::is_partitioned and ranges::partition_point

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I didn't do a detailed analysis of the algorithm used.



================
Comment at: libcxx/include/__algorithm/is_partitioned.h:52
+namespace ranges {
+  struct __is_partitioned_fn final : __function_like {
+    constexpr explicit __is_partitioned_fn(__tag __x) noexcept : __function_like(__x) {}
----------------
Please add `_LIBCPP_TEMPLATE_VIS` and validate all other structs.


================
Comment at: libcxx/include/__algorithm/is_partitioned.h:57
+             indirect_unary_predicate<projected<_Ip, _Proj>> _Pred>
+    _LIBCPP_NODISCARD_EXT constexpr
+    bool operator()(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj = {}) const {
----------------
Please add `_LIBCPP_HIDE_FROM_ABI` and check all other functions.


================
Comment at: libcxx/include/__algorithm/is_partitioned.h:68
+      ++__first;
+      return ranges::find_if(_VSTD::move(__first), _VSTD::move(__last), __pred_ref, __proj_ref) == __last;
+    }
----------------
`__last` is used after moving.


================
Comment at: libcxx/include/__algorithm/partition_point.h:71
+    _LIBCPP_NODISCARD_EXT constexpr
+    _Ip operator()(_Ip __first, const _Sp __last, _Pred __pred, _Proj __proj = {}) const {
+      // This assertion breaks the logarithmic complexity requirement, but since it is only evaluated
----------------
Why the `const` in `const _Sp __last`. (I personally like it, but it deviates from our coding style.)


================
Comment at: libcxx/include/algorithm:369
 
+template<input_iterator I, sentinel_for<I> S, class Proj = identity,
+         indirect_unary_predicate<projected<I, Proj>> Pred>
----------------
Please add `// since C++20` to these two.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_is_partitioned/ranges_is_partitioned.pass.cpp:44
+      auto pred = complexity_invocable(is_odd);
+      auto range = ranges::subrange(I(all_odd.begin()), sentinel_wrapper(all_odd.end()));
+      assert(ranges::is_partitioned(range.begin(), range.end(), std::ref(pred)));
----------------
Can the range move to the parent scope?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_is_partitioned/ranges_is_partitioned.pass.cpp:46
+      assert(ranges::is_partitioned(range.begin(), range.end(), std::ref(pred)));
+      assert(pred.count() <= ranges::ssize(all_odd));
+    }
----------------
Why can't you test against an exact value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105794