ADT: Fix const-correctness of iterator facade

[dexonsmith]

Summary

Fix the const-ness of iterator_facade_base::operator-> and
iterator_facade_base::operator[]. This is a follow-up to
1b651be0465de70cfa22ce4f715d3501a4dcffc1, which fixed const-ness of
various iterator adaptors.

Iterators, like the pointers that they generalize, have two types of
const.

• The const qualifier on members indicates whether the iterator itself can be changed. This is analagous to int *const.
• The const qualifier on return values of operator*(), operator[](), and operator->() controls whether the the pointed-to value can be changed. This is analogous to const int*.

If an iterator facade returns a handle to its own state, then T (and
PointerT and ReferenceT) should usually be const-qualified. Otherwise,
if clients are expected to modify the state itself, the field can be
declared mutable or a const_cast can be used.

Depends on a couple of in-flight patches.

[dblaikie]

Looks good!

[dexonsmith]

Triggering CI again after pushing 9a2b54af221717bf71040b530c4ac8043e1db23a.

[dexonsmith]

Thanks for the reviews! Pushed in 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.

llvm/unittests/ADT/IteratorTest.cpp

85

I ignored this clang-format suggestion since I find &IntIterator::operator->== ... harder to read than &IntIterator::operator-> == ....

[dblaikie]

llvm/unittests/ADT/IteratorTest.cpp

85

Yep, that seems entirely correct - these sort of expressions are uncommon so it's not too surprising that clang-format is misclassifying/misformatting them. (this behavior doesn't seem intentional - it's quite different from how other similar expressions would be formatted that didn't involve naming operator overloads, like x == &y would be formatted with spaces around the == and no space between & and y usually)

But if you wrap the LHS and RHS in () clang-format seems to do OK with it, if you like. No big deal either way, though.

llvm/include/llvm/ADT/iterator.h

 /// always information preserving, and I can see very reasonable iterator
 /// designs where this doesn't work well. It doesn't really force much added
 /// boilerplate anyways.
 ///
 /// Another abstraction that this doesn't provide is implementing increment in
 /// terms of addition of one. These aren't equivalent for all iterator
 /// categories, and respecting that adds a lot of complexity for little gain.
 ///
+/// Iterators are expected to have const rules analogous to pointers, with a
+/// single, const-qualified operator*() that returns ReferenceT. This matches
+/// the second and third pointers in the following example:
+/// \code
+///   int Value;
+///   { int *I = &Value; }             // ReferenceT 'int&'
+///   { int *const I = &Value; }       // ReferenceT 'int&'; const
+///   { const int *I = &Value; }       // ReferenceT 'const int&'
+///   { const int *const I = &Value; } // ReferenceT 'const int&'; const
+/// \endcode
+/// If an iterator facade returns a handle to its own state, then T (and
+/// PointerT and ReferenceT) should usually be const-qualified. Otherwise, if
+/// clients are expected to modify the handle itself, the field can be declared
+/// mutable or use const_cast.
+///
 /// Classes wishing to use `iterator_facade_base` should implement the following
 /// methods:
 ///
 /// Forward Iterators:
 ///   (All of the following methods)
 ///   - DerivedT &operator=(const DerivedT &R);
 ///   - bool operator==(const DerivedT &R) const;
 ///   - T &operator*() const;
#endif
   }
   bool operator>=(const DerivedT &RHS) const {
     static_assert(
         IsRandomAccess,
         "Relational operators are only defined for random access iterators.");
     return !(static_cast<const DerivedT &>(*this) < RHS);
   }
 
-  PointerProxy operator->() {
-    return static_cast<DerivedT *>(this)->operator*();
-  }
   PointerProxy operator->() const {
     return static_cast<const DerivedT *>(this)->operator*();
   }
-  ReferenceProxy operator[](DifferenceTypeT n) {
-    static_assert(IsRandomAccess,
-                  "Subscripting is only defined for random access iterators.");
-    return static_cast<DerivedT *>(this)->operator+(n);
-  }
   ReferenceProxy operator[](DifferenceTypeT n) const {
     static_assert(IsRandomAccess,
                   "Subscripting is only defined for random access iterators.");
     return static_cast<const DerivedT *>(this)->operator+(n);
   }
 };
 
 /// CRTP base class for adapting an iterator to a different type.

llvm/unittests/ADT/IteratorTest.cpp

: public iterator_adaptor_base<PointerProxyWrapper<T, ProxyT>, T *,
                                    ptrdiff_t, T *, ProxyT> {
   PointerProxyWrapper(T *I) : PointerProxyWrapper::iterator_adaptor_base(I) {}
 };
 using IntIterator = PointerWrapper<int>;
 using ConstIntIterator = PointerWrapper<const int>;
 using IntProxyIterator = PointerProxyWrapper<int, IntProxy>;
 using ConstIntProxyIterator = PointerProxyWrapper<const int, ConstIntProxy>;
 
+// There should only be a single (const-qualified) operator*, operator->, and
+// operator[]. This test confirms that there isn't a non-const overload. Rather
+// than adding those, users should double-check that T, PointerT, and ReferenceT
+// have the right constness, and/or make fields mutable.
+static_assert(&IntIterator::operator* == &IntIterator::operator*, "");
+static_assert(&IntIterator::operator-> == &IntIterator::operator->, "");
+static_assert(&IntIterator::operator[] == &IntIterator::operator[], "");
+
 template <class T,
           std::enable_if_t<std::is_assignable<T, int>::value, bool> = false>
 constexpr bool canAssignFromInt(T &&) {
   return true;
 }
 template <class T,
           std::enable_if_t<!std::is_assignable<T, int>::value, bool> = false>
 constexpr bool canAssignFromInt(T &&) {