[libc++] NFCI: Restore code duplication in wrap_iter, with test.

[• Quuxplusone]

Summary

It turns out that D105040 broke `std::rel_ops`; we actually do need
both a one-template-parameter and a two-template-parameter version of
all the comparison operators, because if we have only the heterogeneous
two-parameter version, then `x > x` is ambiguous:

    template<class T, class U> int f(S<T>, S<U>) { return 1; }
    template<class T> int f(T, T) { return 2; }  // rel_ops
    S<int> s; f(s,s);  // ambiguous between #1 and #2

Adding the one-template-parameter version fixes the ambiguity:

    template<class T, class U> int f(S<T>, S<U>) { return 1; }
    template<class T> int f(T, T) { return 2; }  // rel_ops
    template<class T> int f(S<T>, S<T>) { return 3; }
    S<int> s; f(s,s);  // #3 beats both #1 and #2

[bgraur]

The reverted commit introduced an error when std::rel_ops are used.
Here's a small reproducer:

#include <utility> // for std::rel_ops.
#include <vector>

using namespace std::rel_ops;

struct T;
void fn() {
  std::vector<T>::const_iterator a;
  for (std::vector<T>::const_iterator i; i >= a;)
    ;
}

The compilation result:

repro.cc:9:44: error: use of overloaded operator '>=' is ambiguous (with operand types 'std::vector<T>::const_iterator' (aka '__wrap_iter<const T *>') and 'std::vector<T>::const_iterator' (aka '__wrap_iter<const T *>'))
  for (std::vector<T>::const_iterator i; i >= a;)
                                         ~ ^  ~
/google/src/cloud/bgraur/cherrypick/google3/third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/__utility/rel_ops.h:56:1: note: candidate function [with _Tp = std::__wrap_iter<const T *>]
operator>=(const _Tp& __x, const _Tp& __y)
^
/google/src/cloud/bgraur/cherrypick/google3/third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/__iterator/wrap_iter.h:201:6: note: candidate function [with _Iter1 = const T *, _Iter2 = const T *]
bool operator>=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
     ^
1 error generated.

[ldionne]

Thanks a lot for the bug report.

I don't think the Standard requires this to work:

#include <utility> // for std::rel_ops.
#include <vector>

using namespace std::rel_ops;

struct T;
void fn() {
  std::vector<T>::const_iterator a;
  for (std::vector<T>::const_iterator i; i >= a;)
    ;
}

We're allowed to implement vector::iterator however we want as long as it's a LegacyRandomAccessIterator, which includes being able to compare it as x < y, etc. But there is no mention of those comparison operators being required to be injected only when they are used (i.e. implemented as hidden friends).

I'll still try to fix this and add a test (specific to libc++) since this used to work, but I would suggest you try to move away from using std::rel_ops, since it's pretty broken anyway.

[ldionne]

Add test to trigger the std::rel_ops failure.

Arthur kindly suggested to me that he could handle finding the solution, and I will take on that offer so I can keep reviewing ranges stuff. Feel free to commandeer. Thanks!

[• Quuxplusone]

Restore the homogeneous comparison operators. Expand @ldionne's test case a little further. This is ready to land, IMHO, assuming CI is green.

[ldionne]

LGTM once CI passes. Thanks!

[• Quuxplusone]

Disable the new test when <filesystem> is missing.
Refactor a bit.
I was planning to add tests for reverse_iterator comparisons as well, but it turns out that nobody supports that (and neither do we).

[• Quuxplusone]

Rebase on main to re-trigger CI.

[ldionne]

This broke the back-deployment CI: https://buildkite.com/llvm-project/libcxx-ci/builds/4299#fc6f9f98-395a-4b26-a2bf-c5454dcd726c

[• Quuxplusone]

@ldionne: Thanks for fixing that!

libcxx/include/__iterator/wrap_iter.h

 #endif
 
     template <class _Up> friend class __wrap_iter;
     template <class _CharT, class _Traits, class _Alloc> friend class basic_string;
     template <class _Tp, class _Alloc> friend class _LIBCPP_TEMPLATE_VIS vector;
     template <class _Tp, size_t> friend class _LIBCPP_TEMPLATE_VIS span;
 };
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator==(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+    return __x.base() == __y.base();
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator==(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
     return __x.base() == __y.base();
 }
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator<(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+#if _LIBCPP_DEBUG_LEVEL == 2
+    _LIBCPP_ASSERT(__get_const_db()->__less_than_comparable(&__x, &__y),
+                   "Attempted to compare incomparable iterators");
+#endif
+    return __x.base() < __y.base();
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator<(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_ASSERT(__get_const_db()->__less_than_comparable(&__x, &__y),
                    "Attempted to compare incomparable iterators");
 #endif
     return __x.base() < __y.base();
 }
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator!=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+    return !(__x == __y);
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator!=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
     return !(__x == __y);
 }
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator>(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+    return __y < __x;
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator>(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
     return __y < __x;
 }
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator>=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+    return !(__x < __y);
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator>=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
     return !(__x < __y);
 }
 
+template <class _Iter1>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+bool operator<=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
+{
+    return !(__y < __x);
+}
+
 template <class _Iter1, class _Iter2>
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
 bool operator<=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter2>& __y) _NOEXCEPT
 {
     return !(__y < __x);
 }
 
 template <class _Iter1, class _Iter2>

libcxx/test/std/containers/iterator.rel_ops.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: libcpp-has-no-filesystem-library
+
+// Make sure the various containers' iterators are not broken by the use of `std::rel_ops`.
+
+#include <utility> // for std::rel_ops
+
+#include <array>
+#include <deque>
+#include <forward_list>
+#include <list>
+#include <map>
+#include <set>
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+
+#include "test_macros.h"
+
+#if TEST_STD_VER >= 11
+#include "filesystem_include.h"
+#endif
+
+#if TEST_STD_VER >= 17
+#include <string_view>
+#endif
+
+#if TEST_STD_VER >= 20
+#include <span>
+#endif
+
+using namespace std::rel_ops;
+
+template<class It, class ConstIt>
+void test_eq(It it, ConstIt cit) {
+    (void)(it == it);
+    (void)(it != it);
+    (void)(it == cit);
+    (void)(it != cit);
+    (void)(cit == it);
+    (void)(cit != it);
+    (void)(cit == cit);
+    (void)(cit != cit);
+}
+
+template<class It, class ConstIt>
+void test_lt(It it, ConstIt cit) {
+    (void)(it <  it);
+    (void)(it <= it);
+    (void)(it >  it);
+    (void)(it >= it);
+    (void)(it <  cit);
+    (void)(it <= cit);
+    (void)(it >  cit);
+    (void)(it >= cit);
+    (void)(cit <  it);
+    (void)(cit <= it);
+    (void)(cit >  it);
+    (void)(cit >= it);
+    (void)(cit <  cit);
+    (void)(cit <= cit);
+    (void)(cit >  cit);
+    (void)(cit >= cit);
+
+    // Test subtraction too, even though std::rel_ops shouldn't affect it.
+
+    (void)(it - it);
+    (void)(it - cit);
+    (void)(cit - it);
+    (void)(cit - cit);
+}
+
+template<class Container>
+void test_forward() {
+    // There is no need to distinguish "forward" from "bidirectional."
+    // libc++ already can't handle `c.rbegin() >= c.rbegin()` in the
+    // presence of std::rel_ops, and neither can Microsoft nor libstdc++.
+
+    Container c;
+    typename Container::iterator it = c.begin();
+    typename Container::const_iterator cit = c.begin();
+    test_eq(it, cit);
+}
+
+template<class Container>
+void test_random_access() {
+    Container c;
+    typename Container::iterator it = c.begin();
+    typename Container::const_iterator cit = c.begin();
+    test_eq(it, cit);
+    test_lt(it, cit);
+}
+
+template void test_random_access<std::array<int, 10> >();
+template void test_random_access<std::deque<int> >();
+template void test_forward<std::forward_list<int> >();
+template void test_forward<std::list<int> >();
+template void test_forward<std::map<int, int> >();
+template void test_forward<std::multimap<int, int> >();
+template void test_forward<std::multiset<int> >();
+template void test_forward<std::set<int> >();
+template void test_random_access<std::string>();
+template void test_forward<std::unordered_map<int, int> >();
+template void test_forward<std::unordered_multimap<int, int> >();
+template void test_forward<std::unordered_multiset<int> >();
+template void test_forward<std::unordered_set<int> >();
+template void test_random_access<std::vector<int> >();
+
+#if TEST_STD_VER >= 11
+void test_directory_iterators() {
+    fs::directory_iterator it;
+    test_eq(it, it);
+
+    fs::recursive_directory_iterator rdit;
+    test_eq(rdit, rdit);
+}
+
+template void test_forward<fs::path>();
+#endif
+
+#if TEST_STD_VER >= 17
+template void test_random_access<std::string_view>();
+#endif
+
+#if TEST_STD_VER >= 20
+void test_span() {
+    std::span<int> c;
+    std::span<int>::iterator it = c.begin();  // span has no const_iterator
+    test_eq(it, it);
+    test_lt(it, it);
+}
+#endif