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

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
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);
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > (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`.
> As in `find_if_not`'s lambda, you need to std::forward `__e` here. (Economist's $100 bill — be consistent!)
The only requirement on the result of `==` is that it is //`boolean-testable`//, so these lambdas need to explicitly return `bool` - //`boolean-testable`// only guarantees that the exact expression can be used boolean-ish-ly in situ.

> (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.

Since `ranges::equal_to` requires `equality_comparable_with`, this is not observable (we don't say that it compares this way; we say that it returns something for which the comparison yields true if compared that way).

> 
> (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`.

`equality_comparable_with` requires implicit expression variations, so turning into const& should be fine.



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:85
+
+struct NonConstComparableRValue {
+  friend constexpr bool operator==(const NonConstComparableRValue&, const NonConstComparableRValue&) { return false; }
----------------
None of these model `equality_comparable`, so using them with `ranges::find` is IFNDR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248