[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/test/libcxx/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
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure the various container's 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 <unordered_map>
+#include <unordered_set>
+#include <vector>
+
+using namespace std::rel_ops;
+
+template<class Container>
+void test() {
+    Container const c = Container();
+    typename Container::const_iterator it1 = c.end(), it2 = c.end();
+    (void)(it1 == it2);
+    (void)(it1 != it2);
+}
+
+template<class Container>
+void test_random_access() {
+    Container const c = Container();
+    typename Container::const_iterator it1 = c.end(), it2 = c.end();
+    (void)(it1 == it2);
+    (void)(it1 != it2);
+    (void)(it1 < it2);
+    (void)(it1 > it2);
+    (void)(it1 <= it2);
+    (void)(it1 >= it2);
+}
+
+template void test_random_access<std::array<int, 10> >();
+template void test_random_access<std::deque<int> >();
+template void test<std::forward_list<int> >();
+template void test<std::list<int> >();
+template void test<std::map<int, int> >();
+template void test<std::multimap<int, int> >();
+template void test<std::multiset<int> >();
+template void test<std::set<int> >();
+template void test<std::unordered_map<int, int> >();
+template void test<std::unordered_multimap<int, int> >();
+template void test<std::unordered_multiset<int> >();
+template void test<std::unordered_set<int> >();
+template void test_random_access<std::vector<int> >();