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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think it would be fine to have a couple of test cases that use the `complexity_xxx` machinery, but it shouldn't be *all* the test cases. It increases the complexity of understanding the test cases and also we end up only testing with the `complexity_invocable`, not the actual predicates.



================
Comment at: libcxx/include/__algorithm/lower_bound.h:101
+      // useful for catching violated precondition bugs.
+      _LIBCPP_ASSERT(ranges::is_partitioned(__first, __last, _VSTD::ref(__pred), _VSTD::ref(__proj)),
+                     "lower_bound precondition:\n"
----------------
We definitely want to enable this check only when the user requests a stronger level of debugging. It should be possible to enable `_LIBCPP_ASSERT` without violating any complexity guarantee, such that one could in theory enable `_LIBCPP_ASSERT` in the released version of an application. Those expensive checks should be enabled only when a proper debug mode is requested.

However, things are a bit of a mess and we already can't do that today with `_LIBCPP_ASSERT`, so I won't hold you to that here. You can leave as-is, I just wanted to mention it so it's known that we should eventually go back and decide which checks are to be enabled in "debug" mode and which ones when "minimal assertions" are enabled.


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


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/lower.bound/ranges_lower_bound/ranges_lower_bound.pass.cpp:153
+
+    auto const iterator_result = ranges::lower_bound(input.begin(), input.end(), static_cast<T>(2), comp_ref, proj_ref);
+    assert(iterator_result == ranges::next(input.begin(), 4));
----------------
Just `it`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105795