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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM! But please address @var-const's request before landing.



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


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:163
+    assert(std::ranges::binary_search(a, a + 5, 6, Func{}));
+  }
+
----------------
philnik wrote:
> Mordante wrote:
> > Please add some tests for the complexity requirements for comparisons and projections.
> > The same for `lower_bound` and `upper_bound`.
> How exactly would you like them to look like? The standard says that the complexity is `log_2(last - first) + O(1)` How could I (without a statistical analysis) find out what the constant is?
Good point O(1) makes it hard :-( `std::search`'s O(1) can differ from `std::lower_bound`'s O(1).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964