[libcxx-commits] [PATCH] D105795: [libcxx][algorithms] adds ranges::lower_bound and ranges::upper_bound

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/ranges_lower_bound/ranges_lower_bound.pass.cpp:64
+  test_algorithm<I>(no_duplicates, ranges::greater(), [](auto& input, auto& comp) {
+    auto comp_ref = std::ref(comp);
+    assert(ranges::lower_bound(input.begin(), input.end(), static_cast<T>(5), comp_ref) == input.begin());
----------------
Maybe this is redundant with a point @ldionne has already made re `complexity_foo`; but it's important that we test some of these algorithms with predicates that are not `std::reference_wrapper`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/ranges_lower_bound/ranges_lower_bound.pass.cpp:84
+    auto proj_ref = std::ref(proj);
+    auto iterator_result = ranges::lower_bound(input.begin(), input.end(), static_cast<T>(-5), comp_ref, proj_ref);
+    assert(iterator_result == ranges::next(input.begin(), 5));
----------------
miscco wrote:
> ldionne wrote:
> > Here and elsewhere, please make sure you assign the result of `lower_bound` to a variable with a proper type. In the tests, it's useful to pin down the type of things to test that we return the right types (whereas I would agree in normal code `auto` improves readability).
> Actually that would be a bit harmfull due to implicit conversions.
> 
> You should assign to auto and then check the type of the variable via a static assert.
> 
> With concepts you could also do a `same_as<expected_type> auto it = ...`
"A bit harmful" is overstating the case (since that's libc++ style everywhere). But in any test that already requires C++20 Concepts support, I agree:
```
std::same_as<decltype(input.begin())> auto it = ranges::lower_bound(input.begin(), input.end(), ...);
```
would be a pretty neat way to encode the assertion into the test. Let's do that (in tests that already require Concepts, at least).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105795