[libc++] Remove operator-> from iterator archetypes that don't need it

[ldionne]

Summary

operator-> is not a requirement for most iterators, so remove it. To
account for this change, the common_iterator.operator-> test needs to
be refactored quite a bit -- improve test coverage while we're at it.

[• Quuxplusone]

libcxx/test/support/test_iterators.h

255–266

(A) please don't reopen namespace std, but also (B) please don't specialize pointer_traits.
Is your goal just to make contiguous_iterator a contiguous iterator while (for some reason) removing its operator->? You can do that by giving contiguous_iterator a member using element_type = typename pointer_traits<It>::element_type;.
https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator

[ldionne]

libcxx/test/support/test_iterators.h

255–266

(A) please don't reopen namespace std, but also (B) please don't specialize pointer_traits.

Can you please explain why that's bad? That seems gratuitous -- pointer_traits is explicitly made to be customized, no? Arguably, if I could remove all that complexity and only have element_type, that would indeed better, but I don't see why specializing pointer_traits would be bad in itself. Edit: Nope, that doesn't work, I have to keep operator->().

Is your goal just to make contiguous_iterator a contiguous iterator while (for some reason) removing its operator->?

Yes, that's my goal. The reason is to make congituous_iterator more minimal, just like the other changes we did to e.g. cpp17_input_iterator to remove default-constructibility. Because it makes it more useful for testing.

[ldionne]

Upload the patch correctly.

[• Quuxplusone]

I don't agree with this direction, but I think the only hill I'll particularly die on is "don't reopen namespace std."

libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp

28–38

Consider

{
  Common common(iter);
  std::same_as<IteratorWithArrow> auto r1 = static_cast<Common&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r2 = static_cast<Common&&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r3 = static_cast<const Common&>(common).operator->();
  std::same_as<IteratorWithArrow> auto r4 = static_cast<const Common&&>(common).operator->();
  assert(base(r1) == buffer);
}

just to hit the move cases as well. (If IteratorWithArrow were move-only, would there be no operator->? ....waitaminute. You have an infinite loop in your operator->, don't you? IteratorWithArrow::operator->() can't return another IteratorWithArrow! It should be returning int*.)

(And I think I just remembered that common_iterator doesn't work with move-only iterator types, right? because it's meant as an adaptor to produce a C++17 iterator, which must be copyable by definition?)

libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp

94

The code on the left strikes me as weird. But whatever, I'm getting deja vu to a conversation with @CaseyCarter where he said nobody cares about the value of ::pointer. :)

libcxx/test/support/test_iterators.h

255–266

(A) please don't reopen namespace std

https://quuxplusone.github.io/blog/2021/10/27/dont-reopen-namespace-std/

(B) please don't specialize pointer_traits

Because user iterators won't specialize pointer_traits. I suppose there is a tension here between "Keep test code simple; eliminate metaprogramming; make test code realistic" and "Make test code that stress-tests the corner cases." For example, we =delete the comma operator on all of these iterators (and should delete operator& too, probably) — that's more complicated than necessary, and isn't realistic, but it does stress-test the corner cases. So maybe "let's specialize pointer_traits for all these iterators to make sure all our algorithms respect the specialized pointer_traits" is a good suggestion on par with "Let's =delete the comma operator for all these iterators." Personally I don't think it meets the bar; it's a lot of complexity for what it gives us IMO, especially compared to the simplicity of
https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator

[ldionne]

In D118400#3287820, @Quuxplusone wrote:

I don't agree with this direction, but I think the only hill I'll particularly die on is "don't reopen namespace std."

Hopefully we can agree that this is an improvement if I keep operator-> as a way to define contiguous_iterator?

libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp

28–38

Fixed by removing IteratorWithArrow altogether.

And I think I just remembered that common_iterator doesn't work with move-only iterator types, right?

Yes, that's right.

libcxx/test/support/test_iterators.h

255–266

Okay, I sat on this for a while, but I think I agree. In the next iteration I'll remove operator-> only from the archetypes that truly don't need it for anything, like cpp17_input_iterator & friends. I'll keep it on contiguous_iterator.

[ldionne]

Rebase and address review comments.

[• Quuxplusone]

LGTM % the check lambda stuff (which I won't insist on, anyway). Thanks!

libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp

24–41

Nit: Why not good old-fashioned

template<class It>
void test_case_1() {
    // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
    ~~~
}

template<class It>
void test_case_2() {

etc? The whole check.operator()<T>(); thing works, but it doesn't look nice.

libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp

94

For my information, did we ever figure out why the old code thought it'd be const Iter&?
("No" is an acceptable answer. ;))

[ldionne]

libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp

24–41

I actually applied this suggestion, and it resulted in the test case definition and the types it is being called on being further apart, which seemed to decrease legibility. So I'd be in favour of keeping the current approach (even though I agree check.operator()<T>() doesn't look pretty).

libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp

94

Frankly, I did not investigate because int* is what I would have expected all along. However, thinking about it right now:

http://eel.is/c++draft/iterators.common#common.iter.access-5.1 says that we return return get<I>(v_­);, when the iterator has operator->, and that is Iter const& here. When we remove operator-> from the underlying iterator, we instead return addressof(*get<I>(v_)), and that is int*, as we're seeing in the diff after.

libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.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: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // decltype(auto) operator->() const
 //   requires see below;
 
 #include <iterator>
 #include <cassert>
+#include <concepts>
 
+#include "test_iterators.h"
 #include "test_macros.h"
 #include "types.h"
 
 void test() {
+  // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
+  {
+    auto check = []<class Iterator>() {
       int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+      Iterator iter(buffer);
+      using Common = std::common_iterator<Iterator, sentinel_wrapper<Iterator>>;
 
-  // Case 2: http://eel.is/c++draft/iterators.common#common.iter.access-5.2
-  {
-    auto iter1 = simple_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
+      Common common(iter);
+      std::same_as<Iterator> auto result = common.operator->();
+      assert(base(result) == buffer);
+
+      Common const ccommon(iter);
+      std::same_as<Iterator> auto cresult = ccommon.operator->();
+      assert(base(cresult) == buffer);
+    };
 
-    assert(commonIter1.operator->() == buffer);
-    assert(commonIter2.operator->() == buffer);
+    check.operator()<contiguous_iterator<int*>>();
+    check.operator()<int*>();
   }
 
-  // Case 3: http://eel.is/c++draft/iterators.common#common.iter.access-5.3
+  // Case 2: http://eel.is/c++draft/iterators.common#common.iter.access-5.2
   {
-    auto iter1 = value_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
+    auto check = []<class Iterator>() {
+      int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+      Iterator iter(buffer);
+      using Common = std::common_iterator<Iterator, sentinel_type<int*>>;
 
-    assert(*commonIter1.operator->().operator->() == 1);
-    assert(*commonIter2.operator->().operator->() == 1);
+      Common common(iter);
+      std::same_as<int*> auto result = common.operator->();
+      assert(result == buffer);
+
+      Common const ccommon(iter);
+      std::same_as<int*> auto cresult = ccommon.operator->();
+      assert(cresult == buffer);
+    };
+
+    check.operator()<simple_iterator<int*>>();
+    check.operator()<cpp17_input_iterator<int*>>();
+    // cpp20_input_iterator can't be used with common_iterator because it's not copyable
+    check.operator()<forward_iterator<int*>>();
+    check.operator()<bidirectional_iterator<int*>>();
+    check.operator()<random_access_iterator<int*>>();
   }
 
   // Case 3: http://eel.is/c++draft/iterators.common#common.iter.access-5.3
   {
-    auto iter1 = void_plus_plus_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-
-    assert(*commonIter1.operator->().operator->() == 1);
-    assert(*commonIter2.operator->().operator->() == 1);
-  }
-
-  // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
-  {
-    auto iter1 = cpp17_input_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-
-    assert(base(commonIter1.operator->()) == buffer);
-    assert(base(commonIter2.operator->()) == buffer);
-  }
-
-  // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
-  {
-    auto iter1 = forward_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-
-    assert(base(commonIter1.operator->()) == buffer);
-    assert(base(commonIter2.operator->()) == buffer);
-  }
+    auto check = []<class Iterator>() {
+      int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+      Iterator iter(buffer);
+      using Common = std::common_iterator<Iterator, sentinel_type<int*>>;
 
-  // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
-  {
-    auto iter1 = random_access_iterator<int*>(buffer);
-    auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
-    const auto commonIter2 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
+      Common common(iter);
+      auto proxy = common.operator->();
+      std::same_as<int const*> auto result = proxy.operator->();
+      assert(result != buffer); // we copied to a temporary proxy
+      assert(*result == *buffer);
+
+      Common const ccommon(iter);
+      auto cproxy = ccommon.operator->();
+      std::same_as<int const*> auto cresult = cproxy.operator->();
+      assert(cresult != buffer); // we copied to a temporary proxy
+      assert(*cresult == *buffer);
+    };
 
-    assert(base(commonIter1.operator->()) == buffer);
-    assert(base(commonIter2.operator->()) == buffer);
+    check.operator()<value_iterator<int*>>();
+    check.operator()<void_plus_plus_iterator<int*>>();
   }
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }

libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp

// Test with an output_iterator that has a non-void value_type
     using Iter = cpp17_input_iterator<int*>;
     using CommonIter = std::common_iterator<Iter, sentinel_type<int*>>;
     using IterTraits = std::iterator_traits<CommonIter>;
 
     static_assert(std::same_as<IterTraits::iterator_concept, std::input_iterator_tag>);
     static_assert(std::same_as<IterTraits::iterator_category, std::input_iterator_tag>);
     static_assert(std::same_as<IterTraits::value_type, int>);
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
-    static_assert(std::same_as<IterTraits::pointer, const Iter&>);
+    static_assert(std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int&>);
   }
   {
     using Iter = forward_iterator<int*>;
     using CommonIter = std::common_iterator<Iter, sentinel_type<int*>>;
     using IterTraits = std::iterator_traits<CommonIter>;
 
     static_assert(std::same_as<IterTraits::iterator_concept, std::forward_iterator_tag>);
     static_assert(std::same_as<IterTraits::iterator_category, std::forward_iterator_tag>);
     static_assert(std::same_as<IterTraits::value_type, int>);
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
-    static_assert(std::same_as<IterTraits::pointer, const Iter&>);
+    static_assert(std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int&>);
   }
   {
     using Iter = random_access_iterator<int*>;
     using CommonIter = std::common_iterator<Iter, sentinel_type<int*>>;
     using IterTraits = std::iterator_traits<CommonIter>;
 
     static_assert(std::same_as<IterTraits::iterator_concept, std::forward_iterator_tag>);
     static_assert(std::same_as<IterTraits::iterator_category, std::forward_iterator_tag>);
     static_assert(std::same_as<IterTraits::value_type, int>);
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
-    static_assert(std::same_as<IterTraits::pointer, const Iter&>);
+    static_assert(std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int&>);
   }
 
   // Testing iterator conformance.
   {
     static_assert(std::input_iterator<std::common_iterator<cpp17_input_iterator<int*>, sentinel_type<int*>>>);
     static_assert(std::forward_iterator<std::common_iterator<forward_iterator<int*>, sentinel_type<int*>>>);
     static_assert(std::forward_iterator<std::common_iterator<random_access_iterator<int*>, sentinel_type<int*>>>);

libcxx/test/support/test_iterators.h

public:
     typedef It                                                 pointer;
     typedef typename Traits::reference                         reference;
 
     TEST_CONSTEXPR explicit cpp17_input_iterator(It it) : it_(it) {}
     template <class U, class T>
         TEST_CONSTEXPR cpp17_input_iterator(const cpp17_input_iterator<U, T>& u) : it_(u.it_) {}
 
     TEST_CONSTEXPR reference operator*() const {return *it_;}
-    TEST_CONSTEXPR pointer operator->() const {return it_;}
 
     TEST_CONSTEXPR_CXX14 cpp17_input_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 cpp17_input_iterator operator++(int) {return cpp17_input_iterator(it_++);}
 
     friend TEST_CONSTEXPR bool operator==(const cpp17_input_iterator& x, const cpp17_input_iterator& y) {return x.it_ == y.it_;}
     friend TEST_CONSTEXPR bool operator!=(const cpp17_input_iterator& x, const cpp17_input_iterator& y) {return x.it_ != y.it_;}
 
     friend TEST_CONSTEXPR It base(const cpp17_input_iterator& i) { return i.it_; }
public:
     typedef typename std::iterator_traits<It>::reference       reference;
 
     TEST_CONSTEXPR forward_iterator() : it_() {}
     TEST_CONSTEXPR explicit forward_iterator(It it) : it_(it) {}
     template <class U>
         TEST_CONSTEXPR forward_iterator(const forward_iterator<U>& u) : it_(u.it_) {}
 
     TEST_CONSTEXPR reference operator*() const {return *it_;}
-    TEST_CONSTEXPR pointer operator->() const {return it_;}
 
     TEST_CONSTEXPR_CXX14 forward_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 forward_iterator operator++(int) {return forward_iterator(it_++);}
 
     friend TEST_CONSTEXPR bool operator==(const forward_iterator& x, const forward_iterator& y) {return x.it_ == y.it_;}
     friend TEST_CONSTEXPR bool operator!=(const forward_iterator& x, const forward_iterator& y) {return x.it_ != y.it_;}
 
     friend TEST_CONSTEXPR It base(const forward_iterator& i) { return i.it_; }
public:
     typedef typename std::iterator_traits<It>::reference       reference;
 
     TEST_CONSTEXPR bidirectional_iterator() : it_() {}
     TEST_CONSTEXPR explicit bidirectional_iterator(It it) : it_(it) {}
     template <class U>
         TEST_CONSTEXPR bidirectional_iterator(const bidirectional_iterator<U>& u) : it_(u.it_) {}
 
     TEST_CONSTEXPR reference operator*() const {return *it_;}
-    TEST_CONSTEXPR pointer operator->() const {return it_;}
 
     TEST_CONSTEXPR_CXX14 bidirectional_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 bidirectional_iterator& operator--() {--it_; return *this;}
     TEST_CONSTEXPR_CXX14 bidirectional_iterator operator++(int) {return bidirectional_iterator(it_++);}
     TEST_CONSTEXPR_CXX14 bidirectional_iterator operator--(int) {return bidirectional_iterator(it_--);}
 
     friend TEST_CONSTEXPR bool operator==(const bidirectional_iterator& x, const bidirectional_iterator& y) {return x.it_ == y.it_;}
     friend TEST_CONSTEXPR bool operator!=(const bidirectional_iterator& x, const bidirectional_iterator& y) {return x.it_ != y.it_;}
public:
     typedef typename std::iterator_traits<It>::reference       reference;
 
     TEST_CONSTEXPR random_access_iterator() : it_() {}
     TEST_CONSTEXPR explicit random_access_iterator(It it) : it_(it) {}
     template <class U>
         TEST_CONSTEXPR random_access_iterator(const random_access_iterator<U>& u) : it_(u.it_) {}
 
     TEST_CONSTEXPR_CXX14 reference operator*() const {return *it_;}
-    TEST_CONSTEXPR_CXX14 pointer operator->() const {return it_;}
     TEST_CONSTEXPR_CXX14 reference operator[](difference_type n) const {return it_[n];}
 
     TEST_CONSTEXPR_CXX14 random_access_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 random_access_iterator& operator--() {--it_; return *this;}
     TEST_CONSTEXPR_CXX14 random_access_iterator operator++(int) {return random_access_iterator(it_++);}
     TEST_CONSTEXPR_CXX14 random_access_iterator operator--(int) {return random_access_iterator(it_--);}
 
     TEST_CONSTEXPR_CXX14 random_access_iterator& operator+=(difference_type n) {it_ += n; return *this;}
public:
     friend TEST_CONSTEXPR bool operator>=(const contiguous_iterator& x, const contiguous_iterator& y) {return x.it_ >= y.it_;}
 
     friend TEST_CONSTEXPR It base(const contiguous_iterator& i) { return i.it_; }
 
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class It>
 class three_way_contiguous_iterator
 {
     static_assert(std::is_pointer_v<It>, "Things probably break in this case");
 
     It it_;
 
     template <class U> friend class three_way_contiguous_iterator;
 public:
     typedef          std::contiguous_iterator_tag              iterator_category;
     typedef typename std::iterator_traits<It>::value_type      value_type;
     typedef typename std::iterator_traits<It>::difference_type difference_type;
     typedef It                                                 pointer;
     typedef typename std::iterator_traits<It>::reference       reference;
     typedef typename std::remove_pointer<It>::type             element_type;
 
     constexpr It base() const {return it_;}
 
     constexpr three_way_contiguous_iterator() : it_() {}
     constexpr explicit three_way_contiguous_iterator(It it) : it_(it) {}