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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added inline comments.
This revision is now accepted and ready to land.


================
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());
 }
----------------
huixie90 wrote:
> 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
Hmm, using an rvalue reference as the argument type looks promising. The `_Comp_ref` quirk makes things pretty inconvenient, in the future it would be great to clean it up as much as possible without losing the functionality.


================
Comment at: libcxx/include/__algorithm/upper_bound.h:57
+  return std::upper_bound(
+      std::move(__first),
+      std::move(__last),
----------------
@philnik has previously pointed out in one of my patches that this could make the classic algorithm more permissive than mandated by the standard by accepting move-only iterator types. FWIW, I worked around that by adding a `static_assert(is_copy_constructible<>::value)`.

Feel free to address in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129796