[libcxx][test] {move,reverse}_iterator cannot be instantiated for a type with no `operator*` in C++20

[CaseyCarter]

Summary

Since their nested reference types are defined in terms of iter_reference_t<T>, which examines decltype(*declval<T>()).

[• Quuxplusone]

I'd like @ldionne to make an executive decision about whether libc++ should continue to compile move_iterator<ToIter>, as an extension, in pre-C++20 modes. (class ToIter does not have an operator*: dereferencing such a move_iterator wouldn't compile, but merely forming the type is currently legal in libc++ pre-C++20.)
If we think our "extension" is important, then this PR is removing test coverage. If we think no sane user would ever make a move_iterator of a non-iterator type, then this PR is a great simplification.

Scope creep: Same question but in re the fact that ToIter doesn't have an operator++ either. If we discover we also need to add an operator++ to ToIter, would that be removing test coverage?

[CaseyCarter]

In D117371#3246031, @Quuxplusone wrote:

I'd like @ldionne to make an executive decision about whether libc++ should continue to compile move_iterator<ToIter>, as an extension, in pre-C++20 modes. (class ToIter does not have an operator*: dereferencing such a move_iterator wouldn't compile, but merely forming the type is currently legal in libc++ pre-C++20.)
If we think our "extension" is important, then this PR is removing test coverage. If we think no sane user would ever make a move_iterator of a non-iterator type, then this PR is a great simplification.

I'm happy to make the presence of operator* depend on the language mode under test if we do want to preserve this extension, although it seems very low-value to me.

Scope creep: Same question but in re the fact that ToIter doesn't have an operator++ either. If we discover we also need to add an operator++ to ToIter, would that be removing test coverage?

There are a ton of types named "MeowIterator" in the test suite that conform to neither the old iterator requirements nor the new iterator concepts. It's not my goal to fix them all, merely to correct test failures for stdlib=msvc.

[ldionne]

IMO this is a low value extension that we probably never intended to provide, and I think it would be 100% OK to remove that extension. I doubt it would create any downstream breakage too, because nobody should be instantiating move_iterator<It> where It isn't an iterator.

Just curious, why does it not work with MSVC? The most naive implementation of move_iterator would allow instantiating it without an operator* as long as you don't try to instantiate operator*. Does MSVC do it differently?

[• Quuxplusone]

Just curious, why does it not work with MSVC? The most naive implementation of move_iterator would allow instantiating it without an operator* as long as you don't try to instantiate operator*. Does MSVC do it differently?

Casey explains this in the commit summary:

their nested reference types are defined in terms of iter_reference_t<T>, which examines decltype(*declval<T>()).

This is new in C++20; we haven't done it yet (but I'm working on it).

[ldionne]

In D117371#3266376, @Quuxplusone wrote:

Just curious, why does it not work with MSVC? The most naive implementation of move_iterator would allow instantiating it without an operator* as long as you don't try to instantiate operator*. Does MSVC do it differently?

Casey explains this in the commit summary:

their nested reference types are defined in terms of iter_reference_t<T>, which examines decltype(*declval<T>()).

This is new in C++20; we haven't done it yet (but I'm working on it).

Oh thanks for pointing this out and sorry for the noise.

[CaseyCarter]

Oh thanks for pointing this out and sorry for the noise.

Technically the description is incorrect for move_iterator, whose reference is iter_rvalue_reference_t<Iter> which also examines decltype(*declval<Iter>()) indirectly (pun intended) via ranges::iter_move. It is a convenient approximation.

And don't sweat it: I'm renowned for ignoring commit messages, or at least forgetting them at some point during a review.

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp

 struct Base {};
 struct Derived : Base {};
 
 struct ToIter {
     typedef std::forward_iterator_tag iterator_category;
     typedef char *pointer;
     typedef char &reference;
     typedef char value_type;
-    typedef value_type difference_type;
+    typedef signed char difference_type;
 
     explicit TEST_CONSTEXPR_CXX17 ToIter() : m_value(0) {}
     TEST_CONSTEXPR_CXX17 ToIter(const ToIter &src) : m_value(src.m_value) {}
     // Intentionally not defined, must not be called.
     ToIter(char *src);
     TEST_CONSTEXPR_CXX17 ToIter &operator=(char *src) {
         m_value = src;
         return *this;
     }
     TEST_CONSTEXPR_CXX17 ToIter &operator=(const ToIter &src) {
         m_value = src.m_value;
         return *this;
     }
     char *m_value;
+
+    reference operator*() const;
 };
 
 TEST_CONSTEXPR_CXX17 bool test_conv_assign()
 {
     char c = '\0';
     char *fi = &c;
     const std::move_iterator<char *> move_fi(fi);
     std::move_iterator<ToIter> move_ti;

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp

struct Iter {
     using iterator_category = std::bidirectional_iterator_tag;
     using pointer = char*;
     using reference = char&;
     using value_type = char;
     using difference_type = double;
 
     constexpr Iter(double value): m_value(value) {}
     double m_value;
+
+    reference operator*() const;
 private:
     friend constexpr bool operator==(const Iter& l, const Iter& r) = default;
     friend constexpr std::partial_ordering operator<=>(const Iter& l, const Iter& r) = default;
 };
 
 struct ConstIter {
     using iterator_category = std::bidirectional_iterator_tag;
     using pointer = const char*;
     using reference = const char&;
     using value_type = const char;
     using difference_type = double;
 
     constexpr ConstIter(double value): m_value(value) {}
     constexpr ConstIter(Iter it): m_value(it.m_value) {}
     double m_value;
+
+    reference operator*() const;
 private:
     friend constexpr bool operator==(const ConstIter& l, const ConstIter& r) = default;
     friend constexpr std::partial_ordering operator<=>(const ConstIter& l, const ConstIter& r) = default;
 };
 
 constexpr bool tests() {
     char s[] = "1234567890";
     test(three_way_contiguous_iterator<const char*>(s),

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

TEST_CONSTEXPR_CXX17 ToIter &operator=(char *src) {
         m_value = src;
         return *this;
     }
     TEST_CONSTEXPR_CXX17 ToIter &operator=(const ToIter &src) {
         m_value = src.m_value;
         return *this;
     }
     char *m_value;
+
+    reference operator*() const;
 };
 
 TEST_CONSTEXPR_CXX17 bool tests() {
     Derived d;
     test<bidirectional_iterator<Base*> >(bidirectional_iterator<Derived*>(&d));
     test<random_access_iterator<const Base*> >(random_access_iterator<Derived*>(&d));
     test<Base*>(&d);