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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but some remarks.



================
Comment at: libcxx/include/CMakeLists.txt:78
+  __algorithm/ranges_binary_search.h
+  __algorithm/ranges_find.h
+  __algorithm/ranges_find_if.h
----------------
Were these files not already in `CMakeLists.txt`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:62
+      std::same_as<bool> auto ret = std::ranges::binary_search(It(a), It(a + 6), 3);
+      assert(base(ret));
+    }
----------------
` assert(base(ret));` looks wrong, I expect this should be ` assert(ret);`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/binary.search/ranges.binary_search.pass.cpp:163
+    assert(std::ranges::binary_search(a, a + 5, 6, Func{}));
+  }
+
----------------
Please add some tests for the complexity requirements for comparisons and projections.
The same for `lower_bound` and `upper_bound`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121964