[libcxx-commits] [PATCH] D130116: [libc++][ranges] implement `std::ranges::includes`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Almost LGTM -- the only important comment is about sentinel types in tests.



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:32
 Read-only,is_sorted_until,Nikolas Klauser,`D125608 <https://llvm.org/D125608>`_,✅
-Read-only,includes,Nikolas Klauser,n/a,Not started
+Read-only,includes,Hui Xie,n/a,✅
 Read-only,is_heap,Nikolas Klauser,n/a,Not started
----------------
Nit: please include the patch link.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp:48
+      std::ranges::includes(
+          std::forward<Iter1>(iter1),
+          std::forward<Sent1>(sent1),
----------------
Nit: include `<utility>`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp:108
+constexpr void testIncludesImpl(std::array<int, N1> in1, std::array<int, N2> in2, bool expected) {
+  using Sent1 = sentinel_wrapper<In1>;
+  using Sent2 = sentinel_wrapper<In2>;
----------------
I'm a bit concerned that we never check the pretty common case where `In1` and `Sent1` are the same type. Consider making `Sent1` and `Sent2` template parameters. If the test takes too long, I'd rather omit some less interesting iterator types (for `partition_copy`, I only use iterator types with the weakest and the strongest constraints, with the idea that if both `cpp20_input_iterator` and `random_access_iterator` work, then it's very unlikely that `forward_iterator` or `bidirectional_iterator` won't work).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp:296
+
+      constexpr auto comp() {
+        return [this](int x, int y) {
----------------
Optional: there's some prior art to reduce boilerplate here in `test_support/counting_predicates.h`. I used it in https://reviews.llvm.org/D130070 (with some additions) and it seemed to reduce verbosity a little. This can easily be done in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130116