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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
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;
----------------
var-const wrote:
> 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`).
`__make_projected` cannot be used here because the `__comp` takes two arguments but we only want to apply `__proj` to only one of arguments. And tricky thing is sometimes we want to apply `__proj` for the first argument (see line above) and sometimes we want to apply `__proj` to the second argument (see few lines below).

It is just much easier to pass in the `comp` and `proj` and explicitly write how to apply the projection. I will add `static_assert`


================
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));
----------------
philnik wrote:
> var-const wrote:
> > philnik wrote:
> > > var-const wrote:
> > > > 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).
> > > You don't have to (and can't really) pass `_Compare` explicitly. `__lower_bound_impl` always takes the comparator by reference instead of by value. I don't know why this hasn't been done this way before, but that makes a lot more sense that explicitly passing the template parameter around everywhere.
> > I think it was done that way to support `_Comp_ref` resolving to either a reference type (in a regular build) or a non-reference type (in a debug build). Does that not apply here?
> That does apply, but why wouldn't you just pass `_Comp_ref` by reference? The only thing may be that passing it by value is more efficient, but I would expect the functions to be inlined anyways, so it shouldn't matter.
re "Does this need to be forwarded, or not?"
I think it is not. the same expression used the variables twice and you cannot decide which one is executed first so forwarding them just introduces possibility of use-after-move

re ` pass _Compare explicitly`, I agree with @philnik , we shouldn't explicit specify the template argument here and should let it deduced. ` would decay it to a value` is not correct anymore as both functions take arguments by reference.

Actually "explicit specifying the template argument creates problems". My intent of `__upper_bound` is taking comp by forwarding reference, but explicitly specifying the template arguments makes it pass by rvalue reference. (which triggers some ci failure)


================
Comment at: libcxx/include/__algorithm/equal_range.h:54
+  }
+  return pair<_Iter, _Iter>(__first, __first);
 }
----------------
var-const wrote:
> Nit: `make_pair`?
Extra nit nit: `make_pair` in c++03 does extra copies. since it is similar readability, perhaps just leave it as it is? (the original code on the left uses the constructor)


================
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());
 }
----------------
var-const wrote:
> Why is this cast necessary, and does it work in debug mode? (when `_Comp_ref` is a struct)
I added `static_cast` to fix a CI failure but it was in the wrong place. The problem is the one I explained above. In debug mode, the `_Comp_ref` is `debug_less` and the `__equal_range` deduced `_Compare` to `debug_less` . then later, it calls `__upper_bound<policy, _Compare>`
However, `__upper_bound` meant to take `__comp` by forwarding reference `T&&`. But by explicitly specifying the template argument `_Compare`, it changes the meaning and makes it take the rvalue-reference  `debug_less&&`. But in the context of `__equal_range`, `__comp` is an l-value expression of type `debug_less&&`. And an lvalue cannot be bond to an r-value reference input, thus caused failure.

So we should not really explicit specifying template arguments now as most algorithms takes `_Compare` by reference (either lvalue reference or forwarding reference). and explicit writing the type can only cause harm.

It was useful in the past because they used to take `_Compare` by value. But now we are trying our best not to make copies so `_Compare` are mostly passed by references. If the passing-by-reference causes performance regression, we should revisit all the algorithms. But explicit writing the template argument does not help because you cannot stop them being passed by reference

Here I will just remove the explicit template argument and leave the `static_cast`. In normal build it does nothing because it casts an l-value to an lvalue reference. In debug mode, it will construct the temporary `debug_less` struct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129796