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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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);
+  }
----------------
ldionne wrote:
> 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).
@Quuxplusone wants to add the tests in another PR.


================
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
----------------
ldionne wrote:
> This looks weirdly formatted -- please try to indent the return types consistently, etc.
I'm not sure what you are asking for. The return types are all indented the same, and this is copied 1:1 from the standard (excluding the `// since C++20`).


================
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);
+  }
----------------
Quuxplusone wrote:
> 
I get why you want to avoid CTAD in template code, but what's wrong with it here? `std::array {1, 2}` is definitely easier to read than `std::array<int, 2> {1, 2}` and there is no chance that it deduces the wrong type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248