Details
- Reviewers
philnik huixie90 ldionne - Group Reviewers
Restricted Project - Commits
- rG065202f3ca9d: [libc++][ranges] Implement `std::ranges::partition_{point,copy}`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__algorithm/ranges_partition_copy.h | ||
---|---|---|
43 | 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. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
200 | 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) |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
---|---|---|
300 | perhaps also tests "Complexity: Exactly last - first applications of pred and proj." | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_point.pass.cpp | ||
170 | can we have at least one test where the comparator is not is_odd? |
Leaving final approval to @huixie90
libcxx/include/__algorithm/ranges_partition_copy.h | ||
---|---|---|
43 | 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). | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
200 | Yes, I think it's OK to test only the weakest and the strongest iterator types, with a comment. | |
209–211 | Note that I would remove those and replace them by an explanation, otherwise it looks like you simply forgot to uncomment before committing. |
Address feedback, rebase on main.
libcxx/include/__algorithm/ranges_partition_copy.h | ||
---|---|---|
43 | Thanks! Added a TODO. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
300 | I want to consolidate these checks as well, but realistically won't have time before the branch cut. Added the test. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_point.pass.cpp | ||
170 | Done (also in partition and stable_partition). |
LGTM with nits
libcxx/include/__algorithm/ranges_partition_copy.h | ||
---|---|---|
44 | nit: | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
133–134 | 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) | |
143–144 | can we also verify ranges::equal(out1, expected_true) and ranges::equal(out2, expected_false) |
Address feedback.
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp | ||
---|---|---|
133–134 | Very good point re. constexpr. I dropped the checks for all_of altogether in favor of just comparing to expected_* explicitly. | |
143–144 | Thanks for spotting this, this is an oversight on my part. |
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.