[libcxx-commits] [PATCH] D121964: [libc++][ranges] Implement ranges::binary_search and ranges::{lower, upper}_bound

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_upper_bound.h:49
+                                         _Proj __proj = {}) const {
+    auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) { return !std::invoke(__comp, __rhs, __lhs); };
+    return __lower_bound_impl(ranges::begin(__r), ranges::end(__r), __value, __comp_lhs_rhs_swapped, __proj);
----------------
ldionne wrote:
> Should we be forwarding `__lhs` and `__rhs`? I think we're not losing anything if we do it, and I suspect we might be required to (if so, then we're also missing a test case).
`indirect_strict_weak_order` requires that `_Comp` is equality preserving (meaning that cv ref qualifications still give the same output) and it's callable with l- and rvalues, so only the const-ness has to stay (unless I missed some requirement). Because of this I see no reason to make the code harder to read. I think this should actually just be `auto& __lhs, auto& __rhs`.


================
Comment at: libcxx/include/__algorithm/ranges_upper_bound.h:50
+    auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) { return !std::invoke(__comp, __rhs, __lhs); };
+    return __lower_bound_impl(ranges::begin(__r), ranges::end(__r), __value, __comp_lhs_rhs_swapped, __proj);
+  }
----------------
ldionne wrote:
> This is not ADL-proof. Same above. I think we should have ADL proofing tests for ranges algorithms.
The design of `projected` isn't ADL-proof, so we can't add tests. If you want more context see https://wg21.link/P2538.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964