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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

After offline discussions, I'm fine with this approach (I'd like to explore simplifying our implementation of `copy*` and `move*` and potentially removing the internal reverse iterator, but that's for the future and is not guaranteed to work out well). The most important comment I have is to have a lot of context in the class comment to explain the purpose of the class. It's a tricky issue, so we should help the reader of the code as much as we can.



================
Comment at: libcxx/include/__algorithm/copy_backward.h:48
+  auto __reverse_range = std::__reverse_range(std::ranges::subrange(std::move(__first), std::move(__last)));
+  auto __ret           = ranges::copy(std::move(__reverse_range), std::make_reverse_iterator(__result));
+  return std::make_pair(__ret.in.base(), __ret.out.base());
----------------
Question: why doesn't this use `__copy`? (I presume there's a reason, just checking)


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:211
 #if _LIBCPP_STD_VER > 17
     requires requires {
       { __x.base() == __y.base() } -> convertible_to<bool>;
----------------
philnik wrote:
> 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.
Ah, good point.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:338
 template <class _Iter>
-using _ReverseWrapper = reverse_iterator<reverse_iterator<_Iter> >;
+using _AlgRevIter = reverse_iterator<_Iter>;
+#else
----------------
ldionne wrote:
> What do you think about `__unconstrained_reverse_iterator` instead? It's a mouthful, but we don't plan on using it in too many places anyways.
+1 -- there might be a better name, but comparing these two, `__unconstrained_reverse_iterator` makes it easier to understand the purpose of the class.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:341
 
-template <class _Iter, bool __b>
-struct __unwrap_iter_impl<_ReverseWrapper<_Iter>, __b> {
+// reverse_iterator breaks some C++20-hostile iterators. This class is essentially a copy-paste of reverse_iterator
+// with the problematic parts removed to make it usable in the classic STL algorithms in C++20.
----------------
Here's my attempt to improve the wording here. IMO, explaining context here is very important because otherwise it's not clear why we want to maintain essentially two versions of `reverse_iterator`:
"""
This class allows us to use reverse iterators in algorithms' implementation by working around a language issue in C++20.

In C++20, when a reverse iterator wraps certain C++20-"hostile" iterators, calling comparison operators on it will result in a compilation error on Clang. However, calling comparison operators on the pristine hostile iterator is not an error. Thus, we cannot use reverse iterators in the implementation of an algorithm that accepts a hostile iterator. This class is an internal workaround -- it is a copy of `reverse_iterator` with tweaks to make it support hostile iterators.

A "hostile" iterator is one that defines a comparison operator where one of the arguments is an exact match and the other requires an implicit conversion, for example:
```
friend bool operator==(const BaseIter&, const DerivedIter&);
```
C++20 rules for rewriting equality operators create another overload of this function with parameters reversed:
```
friend bool operator==(const DerivedIter&, const BaseIter&);
```
This creates an ambiguity in overload resolution.

However, the ambiguity is only a compilation error when used in a `requires` clause, not in the actual function body. This class simply removes the problematic constraints from comparison functions.
"""


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:402
+  }
+
+
----------------
Nit: there's one extra blank line here, please remove it.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:428
+
+  _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const _AlgRevIter& __lhs, const _AlgRevIter& __rhs) {
+    return __lhs.base() == __rhs.base();
----------------
Consider adding a comment like:
```
// Deliberately unconstrained unlike the comparison functions in `reverse_iterator` -- see the class comment for the rationale.
```


================
Comment at: libcxx/test/libcxx/iterators/predef.iterators/_AlgRevIter/reverse.iter.cmp/equal.pass.cpp:9
+
+// <iterator>
+
----------------
The new reverse iterator is only used in C++20 mode and above, right? Would it make sense to mark these tests as only supporting recent language versions and simplify them (replace `TEST_CONSTEXPR_FOO` with `constexpr`, etc.)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864