[libcxx-commits] [PATCH] D121248: [libc++][ranges] Implement ranges::find{, _if, _if_not}

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a subscriber: tcanens.
Quuxplusone added a comment.

Thanks, LGTM % comments. That was some quick turnaround time!



================
Comment at: libcxx/include/__algorithm/ranges_find.h:39
+  _Ip operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
+    auto __pred = [&](const auto& __e) { return __value == __e; };
+    return __find_if::__fn::__go(std::move(__first), std::move(__last), __pred, __proj);
----------------
(1) https://eel.is/c++draft/algorithms#alg.find-1.4 says `__e == __value`, which agrees with my prejudices too (thankfully). I believe this is observable by the user. Consider adding a regression test.
Ditto throughout.

(2) What you've got now is attractive, but it "flattens" the type of `*__first` into `const auto&`, which might unexpectedly const-qualify and/or lvalue-qualify it, which might(?) affect the resolution of `operator==`. Can you investigate whether this is OK (due to the requirements of `indirect_binary_predicate`)? You might need to use `auto&&` here. (@tcanens, thoughts?)
...Actually, you've already started using `auto&&` in `find_if_not`, implying maybe you found a regression test? Please add a similar test for `find` and `find_if`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:49
+  test_iterators<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
+  test_iterators<forward_iterator<int*>>();
+  test_iterators<random_access_iterator<int*>>();
----------------
Please also test `bidirectional_iterator<int*>`, `int*`, and `const int*`.
Ditto throughout.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:61
+      int a[] = {1, 2, 3, 4};
+      auto ret = std::ranges::find(a, a + 4, a + 3, [](int& i){ return &i; });
+      assert(ret == a + 3);
----------------
IIUC, you meant this one to use the range overload. I wonder if this works, or if it's ambiguous...


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:68
+    [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+      std::ranges::find(std::array {1, 2}, 3);
+  }
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:85
+
+  { // count predicate invocation count
+    {
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if.pass.cpp:34
+    int a[] = {1, 2, 3, 4};
+    std::same_as<It> auto ret = std::ranges::find_if(It(a), Sent(It(a + 4)), [c = 0](int) mutable { return c++ > 2; });
+    assert(base(ret) == a + 3);
----------------
This isn't a valid predicate; also I don't understand the point of doing `c++` here anyway. Are you just trying to make sure the predicate is called non-const? In that case, you could just `[](int x) mutable { return x == 2; }` — the `mutable` makes the type not const-callable.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if.pass.cpp:68
+    [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+      std::ranges::find_if(std::array {1, 2}, [](int){ return false; });
+  }
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if_not.pass.cpp:68
+    [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+      std::ranges::find_if_not(std::array {1, 2}, [](int){ return true; });
+  }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248