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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:5
 Search,none_of,Christopher Di Bella,`D105793 <https://llvm.org/D105793>`_
-Search,find,Christopher Di Bella,`D105456 <https://llvm.org/D105456>`_
-Search,find_if,Christopher Di Bella,`D105792 <https://llvm.org/D105792>`_
-Search,find_if_not,Christopher Di Bella,`D105792 <https://llvm.org/D105792>`_
+Search,find,Nikolas Klauser,`D121248 <https://reviews.llvm.org/D121248>`_, Complete
+Search,find_if,Nikolas Klauser,`D121248 <https://reviews.llvm.org/D121248>`_, Complete
----------------
Ultranit: please use the emoji checkmark, for consistency and better "visibility".


================
Comment at: libcxx/include/__algorithm/ranges_find.h:48
+    auto __pred = [&](auto&& __e) { return __e == __value; };
+    return ranges::__find_if_impl(ranges::begin(__r), ranges::end(__r), __pred, __proj);
+  }
----------------
Question: does `__r` need to be forwarded?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:29
+#include "test_iterators.h"
+
+template <class It, class Sent = It>
----------------
Similar to other algorithm patches, I think this needs more SFINAE tests.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:33
+  {
+    int a[] = {1, 2, 3, 4};
+    std::same_as<It> auto ret = std::ranges::find(It(a), Sent(It(a + 4)), 4);
----------------
Consider adding the following test cases (in all 3 files):
- empty range;
- no element satisfies the predicate;
- all elements satisfy the predicate (also check that it is the first equivalent element that is returned).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_if.pass.cpp:127
+      NonConstComparable a[] = { NonConstComparable{} };
+      auto ret = std::ranges::find(a, a + 1, NonConstComparable{});
+      assert(ret == a);
----------------
This should be `find_if`, right? (same in `find_if_not.pass.cpp`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248