[libcxx-commits] [PATCH] D129796: [libc++][ranges] implement `std::ranges::equal_range`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Thanks!



================
Comment at: libcxx/include/__algorithm/equal_range.h:41
+    _Iter __mid     = _IterOps<_AlgPolicy>::next(__first, __half_len);
+    if (std::__invoke(__comp, std::__invoke(__proj, *__mid), __value)) {
+      __first = ++__mid;
----------------
Why not use `__make_projected`? Wouldn't this cause a visible change in behavior for the classic algorithm (now accepting a pointer-to-member as the comparator)? (alternatively, you could do a `static_assert(__is_callable)` like in e.g. `lower_bound`).


================
Comment at: libcxx/include/__algorithm/equal_range.h:50
+      return pair<_Iter, _Iter>(
+          std::__lower_bound_impl<_AlgPolicy>(__first, __mid, __value, __comp, __proj),
+          std::__upper_bound<_AlgPolicy, _Compare>(++__mp1, __end, __value, __comp, __proj));
----------------
Does this need to be forwarded, or not?


================
Comment at: libcxx/include/__algorithm/equal_range.h:50
+      return pair<_Iter, _Iter>(
+          std::__lower_bound_impl<_AlgPolicy>(__first, __mid, __value, __comp, __proj),
+          std::__upper_bound<_AlgPolicy, _Compare>(++__mp1, __end, __value, __comp, __proj));
----------------
var-const wrote:
> Does this need to be forwarded, or not?
You probably need to pass `_Compare` explicitly as well, I'm not sure why it wasn't done before (`_Compare` is normally a reference, so type deduction would decay it to a value).


================
Comment at: libcxx/include/__algorithm/equal_range.h:54
+  }
+  return pair<_Iter, _Iter>(__first, __first);
 }
----------------
Nit: `make_pair`?


================
Comment at: libcxx/include/__algorithm/equal_range.h:62
+  return std::__equal_range<_ClassicAlgPolicy, _Comp_ref>(
+      std::move(__first), std::move(__last), __value, static_cast<_Comp_ref>(__comp), std::__identity());
 }
----------------
Why is this cast necessary, and does it work in debug mode? (when `_Comp_ref` is a struct)


================
Comment at: libcxx/include/__algorithm/upper_bound.h:48
+upper_bound(_ForwardIterator __first, _ForwardIterator __last, const _Tp& __value, _Compare __comp) {
+  return std::__upper_bound<_ClassicAlgPolicy, _Compare&>(__first, __last, __value, __comp, std::__identity());
 }
----------------
Move?


================
Comment at: libcxx/include/__algorithm/upper_bound.h:57
 template <class _ForwardIterator, class _Tp>
-_LIBCPP_NODISCARD_EXT inline
-_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
Please don't remove the `inline`s -- this could increase binary size. We plan to do some investigation and potentially make a dedicated patch to clean up these unwanted `inline`s.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:40
+    requires(Iter&& iter, Sent&& sent, const T& value, Comp&& comp, Proj&& proj) {
+      std::ranges::equal_range(std::forward<Iter>(iter), std::forward<Sent>(sent), value, comp, proj);
+    };
----------------
Do these have to be forwarded as well?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:55
+template <class R, class T, class Proj = std::identity, class Comp = std::ranges::less>
+concept HasEqualRangeRange =
+    requires(R&& r, const T& value, Comp&& comp, Proj&& proj) {
----------------
Nit: I know it's a pattern, but perhaps we could rename this? `RangeRange` looks a little awkward.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:71
+
+template <class InIter, std::size_t N1>
+constexpr void testEqualRangeImpl(std::array<int, N1>& in, int value, std::ranges::subrange<int*> expected) {
----------------
Nit: `s/N1/N/`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:96
+constexpr void testImpl() {
+  // no match and the searched value is in the middle
+  {
----------------
Nit: consider `s/is in the middle/would be in the middle/`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:120
+
+  // exact one match
+  {
----------------
Ultranit: `s/exact/exactly/`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:164
+    int value = 6;
+    [[maybe_unused]] std::same_as<std::ranges::dangling> decltype(auto) result =
+        std::ranges::equal_range(NonBorrowedRange<InIter>{in.data(), in.size()}, value);
----------------
This can be omitted -- I'll merge the `robust_against_dangling` patch very soon.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.binary.search/equal.range/ranges_equal_range.pass.cpp:195
+    {
+      auto result = std::ranges::equal_range(in, value, [](const Data& x, const Data& y) { return x.data < y.data; });
+
----------------
Optional: share this comparator across the two test cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129796