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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:14
 Search,partition_point,Christopher Di Bella,`D105794 <https://llvm.org/D105794>`_,Under review
-Search,lower_bound,Christopher Di Bella,`D105795 <https://llvm.org/D105795>`_,Under review
-Search,upper_bound,Christopher Di Bella,`D105795 <https://llvm.org/D105795>`_,Under review
+Search,lower_bound,Nikolas Klauser,,✅
+Search,upper_bound,Nikolas Klauser,,✅
----------------
Can you please add a link to this patch?


================
Comment at: libcxx/include/__algorithm/ranges_lower_bound.h:36
+
+template <class _Ip, class _Sp, class _Tp, class _Proj, class _Comp>
+_LIBCPP_HIDE_FROM_ABI constexpr
----------------
I think for consistency with other patches, it should use longer names (`_Iterator`, etc.).


================
Comment at: libcxx/include/__algorithm/ranges_lower_bound.h:41
+  __diff_t __len = std::distance(__first, __last);
+  while (__len != 0) {
+    __diff_t __len2 = std::__half_positive(__len);
----------------
Nit: can you split this function into ~3 logical blocks? I find it a little hard(er) to read as a monolithic block of code. I'd suggest a blank line before `while` (to separate the initial setup from the main part), before the `return` statement (similarly to make the main part stand out), and either before or after the call to `std::advance`.


================
Comment at: libcxx/include/__algorithm/ranges_upper_bound.h:38
+  _Ip operator()(_Ip __first, _Sp __last, const _Tp& __value, _Comp __comp = {}, _Proj __proj = {}) const {
+    auto __comp2 = [&](auto&& __lhs, auto&& __rhs) { return !std::invoke(__comp, __rhs, __lhs); };
+    return __lower_bound_impl(__first, __last, __value, __comp2, __proj);
----------------
Nit: similar to another patch, can you name this lambda something like `__comp_lhs_rhs_swapped`? (or just `__comp_swapped`)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:2
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
(please note that many comments in this file apply to other test files as well)


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


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:132
+
+  { // check that an empty range works
+    {
----------------
I'd add tests for:
- 1 element range (too small to meaningfully divide in half);
- even number of elements;
- odd number of elements;
- the searched-for element is the first one;
- the searched-for element is the last one;
- all elements other than the searched-for element are equal.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:177
+  static_assert(test());
+}
----------------
Please add `return 0`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/upper.bound/ranges.upper_bound.pass.cpp:65
+    auto range = std::ranges::subrange(It(a), It(a + 6));
+    auto ret = std::ranges::upper_bound(range, 3);
+    assert(base(ret) == a + 3);
----------------
Optional: perhaps check the return type here as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964