[libcxx-commits] [PATCH] D128864: [libc++] Fix algorithms which use reverse_iterator

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D128864#3667664 <https://reviews.llvm.org/D128864#3667664>, @var-const wrote:

> @philnik My initial impression is that we shouldn't try to solve this at the library level. This looks like a legitimate issue with the core language.

I agree that this is a core issue, but not fixing it is also not an option IMO.

> If I understand it correctly (and please point out if I'm wrong), the issue is that there is no perfect match for `operator==`, and the only `operator==` available has essentially this form:
>
>   bool operator==(const BaseIter&, const DerivedIter&);
>
> Which would work except that the C++20 rewriting rules also generate the same function with reversed arguments:
>
>   bool operator==(const DerivedIter&, const BaseIter&);
>
> Which then becomes ambiguous.

Yes.

> I think it would be easier to rewrite our existing algorithms to avoid using `reverse_iterator`. We can add a TODO to revisit the implementation when (and if) the core language issue gets fixed.

I don't think that it would be easier two rewrite existing algorithms. This would lead to a lot of complicated duplicated code. That is always a recipe for chaos and divergence. Effectively duplicating `reverse_iterator` isn't perfect either, but the code really isn't complicated. Most member functions are a single line.



================
Comment at: libcxx/include/__iterator/reverse_iterator.h:211
 #if _LIBCPP_STD_VER > 17
     requires requires {
       { __x.base() == __y.base() } -> convertible_to<bool>;
----------------
var-const wrote:
> If we're doing essentially a workaround, would it be possible/easier to instead define an internal comparator for reverse iterators and use it in the affected algorithms?
> ```
> bool __reverse_iter_eq(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y) {
>   return __x.base() == __y.base();
> }
> // In the algorithm
> for (; !std::__reverse_iter_eq(__first, __last); ++__first) {
> ```
Not really. There would be no easy way to distinguish between an internal `reverse_iterator` and an external one.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:429
+  _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const _AlgRevIter& __lhs, const _AlgRevIter& __rhs) {
+    return __lhs.base() == __rhs.base();
+  }
----------------
var-const wrote:
> Hmm, so this expression is not ambiguous, but when it's used as a constraint in a `requires` clause, it is considered ambiguous? Am I missing something?
No, that's exactly the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864