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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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);
----------------
tcanens wrote:
> 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.
> 
@philnik We need to address the `-> bool` in the lambda.

You should add a test: `operator==` returns something that can be converted to `bool`, but that can't be moved or copied. With the current implementation, it should fail cause you're returning it from the lambda. If you switch to `-> bool`, there would be an implicit conversion to `bool` and it should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248