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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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
----------------
cjdb wrote:
> 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.)
I like the `const` too for the the same reason. So then let's keep it. I probably will start to use it in new code too.


================
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));
+    }
----------------
cjdb wrote:
> 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)`?
No I like the `ssize(all_odd)` over `5`. IMO `5` is a fragile magic number.
I see I misread the test. It tests the complexity of the algorithm.
IMO it would be clearer to use `assert(pred.count() <= ranges::ssize(range));`

So just discard my previous comment.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105794