[llvm] 6b9b86d - ADT: Fix const-correctness of iterator facade

[Duncan P. N. Exon Smith via llvm-commits]

Author: Duncan P. N. Exon Smith
Date: 2021-11-12T20:41:47-08:00
New Revision: 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c

URL: https://github.com/llvm/llvm-project/commit/6b9b86db9dd974585a5c71cf2e5231d1e3385f7c
DIFF: https://github.com/llvm/llvm-project/commit/6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.diff

LOG: ADT: Fix const-correctness of iterator facade

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.

Added: 
    

Modified: 
    llvm/include/llvm/ADT/iterator.h
    llvm/unittests/ADT/IteratorTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/iterator.h b/llvm/include/llvm/ADT/iterator.h
index 070320581a71c..6f0c42fe08bec 100644
--- a/llvm/include/llvm/ADT/iterator.h
+++ b/llvm/include/llvm/ADT/iterator.h
@@ -35,6 +35,21 @@ namespace llvm {
 /// 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:
 ///
@@ -187,17 +202,9 @@ class iterator_facade_base {
     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.");

diff  --git a/llvm/unittests/ADT/IteratorTest.cpp b/llvm/unittests/ADT/IteratorTest.cpp
index 4e69ad28aabe0..ba92d00d7d3df 100644
--- a/llvm/unittests/ADT/IteratorTest.cpp
+++ b/llvm/unittests/ADT/IteratorTest.cpp
@@ -78,6 +78,14 @@ 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 &&) {