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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:5-7
+Search,find,Nikolas Klauser,n/a, Complete
+Search,find_if,Nikolas Klauser,n/a, Complete
+Search,find_if_not,Nikolas Klauser,n/a, Complete
----------------



================
Comment at: libcxx/include/__algorithm/ranges_find.h:48
+    auto __pred = [&](const auto& __e) { return __value == __e; };
+    return __find_if::__fn::__go(ranges::begin(__r), ranges::end(__r), __pred, __proj);
+  }
----------------
I don't like that we are reaching into `__find_if::__fn` here. Instead, we should have something like this in `find_if.h`:

```
template <class _Ip, class _Sp, class _Pred, class _Proj>
_LIBCPP_HIDE_FROM_ABI constexpr
_Ip __find_if_impl(_Ip __first, _Sp __last, _Pred& __pred, _Proj& __proj) {
    for (; __first != __last; ++__first) {
        if (std::invoke(__pred, std::invoke(__proj, *__first)))
            break;
    }
    return __first;
}
```

This provides a slightly (but not much) cleaner interface between the two headers, at least. TBH, I would support implementing this as `ranges::find_if(ranges::begin(__r), ranges::end(__r), __pred, std::move(__proj));` (and similarly for the iterator/sentinel version), even though I understand this will lead to additional moves over the current approach. In practice, I don't think this is likely to matter, since iterators should be cheap to copy (and hence to move), and same for projections.

So, I have a slight preference for implementation simplicity despite it causing additional moves, but if you want to keep it as-is, please use a namespace scope function at least (and add a libc++ specific test for the no-move behavior).


================
Comment at: libcxx/include/algorithm:48-71
+  template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
+    requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
+    constexpr I find(I first, S last, const T& value, Proj proj = {});              // since C++20
+  template<input_range R, class T, class Proj = identity>
+    requires indirect_binary_predicate<ranges::equal_to, projected<iterator_t<R>, Proj>, const T*>
+    constexpr borrowed_iterator_t<R>
+      find(R&& r, const T& value, Proj proj = {});                                  // since C++20
----------------
This looks weirdly formatted -- please try to indent the return types consistently, etc.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:54-58
+    {
+      int a[] = {1, 2, 3, 4};
+      auto ret = std::ranges::find(a, a + 4, a + 3, [](int& i){ return &i; });
+      assert(ret == a + 3);
+    }
----------------
These projection tests are really clever. Something like this would be easier to read:

```
struct Point { int x; int y; };
Point points[] = {....};
auto ret = std::ranges::find(points, points + 4, 3, [](Point const& p) { return p.x; });
assert(ret == ....);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248