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

Wed Mar 9 08:43:20 PST 2022

Quuxplusone added inline comments.

================
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);
+  }
----------------
philnik wrote:
> var-const wrote:
> > Question: does `__r` need to be forwarded?
> Definitely not the first one, and I also don't think the second one. We still need our range for the iterators to be valid, so conceptually it doesn't make sense to forward them (and makes probably no practical difference).
Right, you never forward ranges. The Ranges idiom is "Take by forwarding reference, pass by lvalue." Because Ranges uses rvalueness to signify //short lifetime//, rather than //pilferability//; see
https://quuxplusone.github.io/blog/2022/02/02/look-what-they-need/#if-you-see-code-using-std-forwar
(See also Tristan Brindle's response https://tristanbrindle.com/posts/ranges-and-forwarding-references , which is kind of a prequel to that sentence of my post: it explains that Ranges uses constness to signify //nothing// rather than //not-gonna-modify-it-ness//, which explains why non-modifying algorithms can't just use `const T&` and have to use `T&&` in the first place.)

================
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:
> (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!)

================
Comment at: libcxx/include/__algorithm/ranges_find_if.h:32-40
+template <class _Ip, class _Sp, class _Pred, class _Proj>
+_LIBCPP_HIDE_FROM_ABI static 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;
+  }
----------------
(1) @ldionne, I disagree that this should be a standalone free function. Reaching into `ranges::__foobar::__fn::__go` doesn't strike me as any more confusing than reaching into `ranges::__foobar_impl`; all it does is add yet another layer of indirection for the gamedevs to complain about. It also implies code churn: as we implement new algorithms in terms of older ones, you're asking us to go back and pull out the //old// algorithm's guts from the existing `__fn::__go` into `__foobar_impl`. (I don't think you're asking for the pulling-out to be done preemptively on //every// algorithm, are you? It would be ad-hoc?) In short, I fairly strongly recommend putting this back the way it was; but I think @ldionne has the final say, so my job is to convince him. :)

(2) If this is kept: please add a blank line after `namespace ranges {` for readability.

(3) In fact, I would even //recommend// closing and reopening the namespace. My mental model is that we have this one helper in namespace `ranges`, and then further down in the file we have the essentially unrelated stuff in namespace `ranges::__find_if`. "Zeroing" in between is good practice IMHO: it makes it easier to see at a glance what namespace we're inside, because you don't have to keep a mental stack of the //entire// file but only back as far as the last "zero" point.

================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:61
+      int a[] = {1, 2, 3, 4};
+      auto ret = std::ranges::find(a, a + 4, a + 3, [](int& i){ return &i; });
+      assert(ret == a + 3);
----------------
Quuxplusone wrote:
> IIUC, you meant this one to use the range overload. I wonder if this works, or if it's ambiguous...
For the record, `ranges::find(a, a+3, proj)` is //not// ambiguous, because `proj` is not equality-comparable-with `int`. But if you make it equality-comparable...
https://godbolt.org/z/88K1d1PKh
(Notice the non-explicit `operator bool` here; that's a code smell. So this isn't a real pitfall for users.)
@philnik, I think it would be cute, and perhaps even useful, to add this godbolt as a test case. It does probably somewhat constrain our possible implementation strategies for this overload set.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121248