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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with my comments addressed. I don't think I need to see this again whatever you do for the `auto const&` issue.



================
Comment at: libcxx/include/__algorithm/equal_range.h:54
                    (
-                      _VSTD::__lower_bound<_Compare>(__first, __m, __value_, __comp),
+                      _VSTD::__lower_bound_impl(__first, __m, __value_, __comp, __proj),
                       _VSTD::__upper_bound<_Compare>(++__mp1, __last, __value_, __comp)
----------------
Can you please make sure that we have a test for the STL classic algorithms that will fail if you make a copy of `__comp` here? You can just add `auto __dummy = __comp; (void)__dummy;` to test it.


================
Comment at: libcxx/include/__algorithm/lower_bound.h:47-48
+_ForwardIterator lower_bound(_ForwardIterator __first, _ForwardIterator __last, const _Tp& __value_, _Compare __comp) {
+  static_assert(__is_callable<_Compare, decltype(*__first), const _Tp&>::value,
+                "The comparator has to be callable");
+  auto __proj = std::__identity();
----------------
Can you please add a libc++ specific test to check that we diagnose if you pass something that isn't a proper function object to `lower_bound` (and the other algorithms in this patch too), please?


================
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);
----------------
philnik wrote:
> philnik wrote:
> > 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`.
> Ignore the last part. Using `auto&` would just make the implementation weirder.
If it's true that cv-qualification doesn't matter, perhaps we should make this `auto const&` instead? That definitely removes a mental burden when reading this sort of code, IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964