[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_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;
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:71
+
+constexpr bool test() {
+  test_iterators<int*>();
----------------
var-const wrote:
> Similar to another patch, it probably makes sense to test that the comparator works when:
> - it takes the arguments by non-const lvalue;
> - its `operator()` is not `const`;
> - it returns something convertible to `bool`.
I'll add the convertible to `bool` tests once D122011 landed. Taking only non-`const` lvalues doesn't model `indirect_strict_weak_order`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964