[PATCH] D113799: ADT: Fix const-correctness of iterator facade

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

dexonsmith updated this revision to Diff 386958.
dexonsmith added a comment.

Triggering CI again after pushing 9a2b54af221717bf71040b530c4ac8043e1db23a <https://reviews.llvm.org/rG9a2b54af221717bf71040b530c4ac8043e1db23a>.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113799/new/

https://reviews.llvm.org/D113799

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


Index: llvm/unittests/ADT/IteratorTest.cpp
===================================================================
--- llvm/unittests/ADT/IteratorTest.cpp
+++ llvm/unittests/ADT/IteratorTest.cpp
@@ -78,6 +78,14 @@
 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 &&) {
Index: llvm/include/llvm/ADT/iterator.h
===================================================================
--- llvm/include/llvm/ADT/iterator.h
+++ llvm/include/llvm/ADT/iterator.h
@@ -35,6 +35,21 @@
 /// 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 @@
     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.");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D113799.386958.patch
Type: text/x-patch
Size: 2956 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211112/97341ec7/attachment.bin>