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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Sorry for being MIA, but I do have a few comments. Some of them may apply to algorithms that we've shipped previously as well.



================
Comment at: libcxx/include/__algorithm/ranges_lower_bound.h:38
+_LIBCPP_HIDE_FROM_ABI constexpr
+_Ip __lower_bound_impl(_Ip __first, _Sp __last, const _Tp& __value, _Comp& __comp, _Proj& __proj) {
+  using __diff_t = typename iterator_traits<_Ip>::difference_type;
----------------
Mordante wrote:
> philnik wrote:
> > var-const wrote:
> > > Have you considered sharing the implementation of the algorithm between the ranges and non-ranges versions? This algorithm, while not very complicated, isn't trivial and is quite prone to off-by-one errors. Personally, I'm on the fence.
> > > 
> > > (cc'ing @ldionne as well)
> > Yes, but I think it's simple enough to not warrant sharing the implementation, but I'm on the fence too.
> I don't have a strong opinion whether to combine these two versions or keep them separate. `std::invoke` requires C++17, but our implementation of `std::invoke` uses `std::__invoke` requires C++11. So it seems to be an issue to use invoke on C++03.
I would really like to share this with the implementation in `<__algorithm/__lower_bound.h>`. I suspect it's not too difficult to do that, and we'll avoid duplicating code.


================
Comment at: libcxx/include/__algorithm/ranges_lower_bound.h:40
+  using __diff_t = typename iterator_traits<_Iter>::difference_type;
+  __diff_t __len = std::distance(__first, __last);
+
----------------
Shouldn't this be `ranges::distance`?


================
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);
----------------
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).


================
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);
+  }
----------------
This is not ADL-proof. Same above. I think we should have ADL proofing tests for ranges algorithms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964