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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D105794#2893803 <https://reviews.llvm.org/D105794#2893803>, @Mordante wrote:

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

Thanks for the review! A few clarifying things, but this all sounds reasonable.



================
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
----------------
Mordante wrote:
> Why the `const` in `const _Sp __last`. (I personally like it, but it deviates from our coding style.)
I like the compiler catching accidental writes. Also, this is a new section of code, so I think it's worth piloting this style here. (@ldionne seems to be ambivalent towards it based on prior commits as evidence.)


================
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)));
----------------
Mordante wrote:
> Can the range move to the parent scope?
Yes, it can. I think this is a relic of when `range` used `complexity_iterator` (where the answer would've been "no").


================
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));
+    }
----------------
Mordante wrote:
> Why can't you test against an exact value?
I guess it's possible to say `pred.count() <= 5`, but my interpretation of the wording at the time made me want to base this in terms of the range's length. I take it you'd prefer `5` over `ssize(all_odd)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105794