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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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.
----------------
var-const wrote:
> 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.
> """
> However, the ambiguity is only a compilation error when used in a requires clause, not in the actual function body.
This sentence would be better as:
"""
Confusingly, Clang treats this ambiguity differently in different contexts. When `operator==` is actually called in the function body, the code is accepted with a warning. When a concept requires `operator==` to be a valid expression, however, it evaluates to `false`. Thus, the implementation of `reverse_iterator::operator==` can actually call `operator==` on its base iterators, but the constraints on `reverse_iterator::operator==` prevent it from being considered during overload resolution. This class simply removes the problematic constraints from comparison functions.
"""


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128864