[libcxx-commits] [PATCH] D130070: [libc++][ranges] Implement `std::ranges::partition_{point, copy}`.

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision.
huixie90 added a comment.
This revision is now accepted and ready to land.

LGTM with nits



================
Comment at: libcxx/include/__algorithm/ranges_partition_copy.h:44
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  static partition_copy_result<_InIter, _OutIter1, _OutIter2> __partition_copy_fn_impl(
+      _InIter&& __first, _Sent&& __last, _OutIter1&& __out_true, _OutIter2&& __out_false,
----------------
nit:
`_InIter` can be a reference type if we don't do `std::move` on the caller. Since this is a local static function, it is unlikely to be used elsewhere. so it is ok to leave it as it is. If this were an `__` function, I would either uncvref them, or pass them by value to make sure we have a `partition_copy_result` of value types


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp:133-134
+  { // (iterator, sentinel) overload.
+    std::array<int, N1> out1; // Allocate a bigger size in case of buffer overflow.
+    std::array<int, N1> out2;
+
----------------
nit: It is OK to allocate `N2` and `N3` I think. In case the algorithm goes wrong and cause buffer overflow, the `constexpr` test would fail. It also simplifies the verification to just `ranges::all_of(out1, pred)`


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp:143-144
+
+    assert(std::ranges::all_of(out1.begin(), base(result.out1), pred));
+    assert(std::ranges::all_of(out2.begin(), base(result.out2), neg_pred));
+  }
----------------
can we also verify `ranges::equal(out1, expected_true)`  and `ranges::equal(out2, expected_false)` 
otherwise the test would still pass if the algorithm simply repeatedly copies the first element  in the group


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130070