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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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;
+
----------------
huixie90 wrote:
> 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)`
Very good point re. `constexpr`. I dropped the checks for `all_of` altogether in favor of just comparing to `expected_*` explicitly.


================
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));
+  }
----------------
huixie90 wrote:
> 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
Thanks for spotting this, this is an oversight on my part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130070