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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

Leaving final approval to @huixie90



================
Comment at: libcxx/include/__algorithm/ranges_partition_copy.h:43
+  _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,
----------------
var-const wrote:
> These two algorithms seem borderline when choosing whether to delegate or reimplement. I'm quite open to delegate if you feel it would be beneficial.
I would have a preference for delegating. I'm also fine with a TODO to do it after the release, since you have it implemented right now and it works (according to the tests).


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp:207-209
+  //test_iterators_in_sent_out1<InIter, Sent, forward_iterator<int*>>();
+  //test_iterators_in_sent_out1<InIter, Sent, random_access_iterator<int*>>();
+  //test_iterators_in_sent_out1<InIter, Sent, contiguous_iterator<int*>>();
----------------
Note that I would remove those and replace them by an explanation, otherwise it looks like you simply forgot to uncomment before committing.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp:198
+  test_iterators_in_sent_out1_out2<InIter, Sent, Out1, cpp20_output_iterator<int*>>();
+  //test_iterators_in_sent_out1_out2<InIter, Sent, Out1, random_access_iterator<int*>>();
+  //test_iterators_in_sent_out1_out2<InIter, Sent, Out1, contiguous_iterator<int*>>();
----------------
var-const wrote:
> I am concerned about the combinatorial explosion here. Perhaps it is sufficient to test with just the weakest and the "strongest" iterator types that satisfy the necessary constraints?
> 
> (Note that I think `partition_copy` is the only algorithm to have two different output iterator types)
Yes, I think it's OK to test only the weakest and the strongest iterator types, with a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130070