ADT: Fix const-correctness of iterator adaptors

This fixes const-correctness of iterator adaptors, dropping non-`const`
overloads for `operator*()`.

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 *`.

Since `operator*()` does not (in principle) change the iterator, then
there should only be one definition, which is `const`-qualified. E.g.,
iterators wrapping `int*` should look like:
```
int *operator*() const; // always const-qualified, no overloads
```

ba7a6b3 changed `iterator_adaptor_base`
away from this to work around bugs in other iterator adaptors. That was
already reverted. This patch adds back its test, which combined
llvm::enumerate() and llvm::make_filter_range(), adds a test for
iterator_adaptor_base itself, and cleans up the `const`-ness of the
other iterator adaptors.

This also updates the documented requirements for
`iterator_facade_base`:
```
/// OLD:
///   - const T &operator*() const;
///   - T &operator*();

/// New:
///   - T &operator*() const;
```
In a future commit we might also clean up `iterator_facade`'s overloads
of `operator->()` and `operator[]()`. These already (correctly) return
non-`const` proxies regardless of the iterator's `const` qualifier.

Differential Revision: https://reviews.llvm.org/D113158

Author: dexonsmith

llvm/include/llvm/ADT/STLExtras.h

@@ -671,9 +671,7 @@ struct zip_common : public zip_traits<ZipType, Iters...> {
 public:
   zip_common(Iters &&... ts) : iterators(std::forward<Iters>(ts)...) {}
 
-  value_type operator*() { return deref(std::index_sequence_for<Iters...>{}); }
-
-  const value_type operator*() const {
+  value_type operator*() const {
     return deref(std::index_sequence_for<Iters...>{});
   }
 
@@ -844,8 +842,6 @@ class zip_longest_iterator
       : iterators(std::forward<Iters>(ts.first)...),
         end_iterators(std::forward<Iters>(ts.second)...) {}
 
-  value_type operator*() { return deref(std::index_sequence_for<Iters...>{}); }
-
   value_type operator*() const {
     return deref(std::index_sequence_for<Iters...>{});
   }
@@ -1939,8 +1935,7 @@ template <typename R> struct result_pair {
   }
 
   std::size_t index() const { return Index; }
-  const value_reference value() const { return *Iter; }
-  value_reference value() { return *Iter; }
+  value_reference value() const { return *Iter; }
 
 private:
   std::size_t Index = std::numeric_limits<std::size_t>::max();
@@ -1949,11 +1944,8 @@ template <typename R> struct result_pair {
 
 template <typename R>
 class enumerator_iter
-    : public iterator_facade_base<
-          enumerator_iter<R>, std::forward_iterator_tag, result_pair<R>,
-          typename std::iterator_traits<IterOfRange<R>>::difference_type,
-          typename std::iterator_traits<IterOfRange<R>>::pointer,
-          typename std::iterator_traits<IterOfRange<R>>::reference> {
+    : public iterator_facade_base<enumerator_iter<R>, std::forward_iterator_tag,
+                                  const result_pair<R>> {
   using result_type = result_pair<R>;
 
 public:
@@ -1963,7 +1955,6 @@ class enumerator_iter
   enumerator_iter(std::size_t Index, IterOfRange<R> Iter)
       : Result(Index, Iter) {}
 
-  result_type &operator*() { return Result; }
   const result_type &operator*() const { return Result; }
 
   enumerator_iter &operator++() {

llvm/include/llvm/ADT/iterator.h

@@ -42,8 +42,7 @@ namespace llvm {
 ///   (All of the following methods)
 ///   - DerivedT &operator=(const DerivedT &R);
 ///   - bool operator==(const DerivedT &R) const;
-///   - const T &operator*() const;
-///   - T &operator*();
+///   - T &operator*() const;
 ///   - DerivedT &operator++();
 ///
 /// Bidirectional Iterators:
@@ -348,8 +347,7 @@ class pointer_iterator
   explicit pointer_iterator(WrappedIteratorT u)
       : pointer_iterator::iterator_adaptor_base(std::move(u)) {}
 
-  T &operator*() { return Ptr = &*this->I; }
-  const T &operator*() const { return Ptr = &*this->I; }
+  T &operator*() const { return Ptr = &*this->I; }
 };
 
 template <typename RangeT, typename WrappedIteratorT =

llvm/unittests/ADT/IteratorTest.cpp

@@ -52,6 +52,90 @@ using IsAdaptedIterCategorySame =
   std::is_same<typename std::iterator_traits<It>::iterator_category,
                typename std::iterator_traits<A<It>>::iterator_category>;
 
+// Check that dereferencing works correctly adapting pointers and proxies.
+template <class T>
+struct PointerWrapper : public iterator_adaptor_base<PointerWrapper<T>, T *> {
+  PointerWrapper(T *I) : iterator_adaptor_base<PointerWrapper, T *>(I) {}
+};
+struct IntProxy {
+  int &I;
+  IntProxy(int &I) : I(I) {}
+  void operator=(int NewValue) { I = NewValue; }
+};
+struct ConstIntProxy {
+  const int &I;
+  ConstIntProxy(const int &I) : I(I) {}
+};
+template <class T, class ProxyT>
+struct PointerProxyWrapper
+    : public iterator_adaptor_base<PointerProxyWrapper<T, ProxyT>, T *,
+                                   std::random_access_iterator_tag, T,
+                                   ptrdiff_t, T *, ProxyT> {
+  PointerProxyWrapper(T *I)
+      : iterator_adaptor_base<PointerProxyWrapper, T *,
+                              std::random_access_iterator_tag, T, ptrdiff_t,
+                              T *, ProxyT>(I) {}
+};
+using IntIterator = PointerWrapper<int>;
+using ConstIntIterator = PointerWrapper<const int>;
+using IntProxyIterator = PointerProxyWrapper<int, IntProxy>;
+using ConstIntProxyIterator = PointerProxyWrapper<const int, ConstIntProxy>;
+
+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 &&) {
+  return false;
+}
+
+TEST(IteratorAdaptorTest, Dereference) {
+  int Number = 1;
+
+  // Construct some iterators and check whether they can be assigned to.
+  IntIterator I(&Number);
+  const IntIterator IC(&Number);
+  ConstIntIterator CI(&Number);
+  const ConstIntIterator CIC(&Number);
+  EXPECT_EQ(true, canAssignFromInt(*I));    // int *
+  EXPECT_EQ(true, canAssignFromInt(*IC));   // int *const
+  EXPECT_EQ(false, canAssignFromInt(*CI));  // const int *
+  EXPECT_EQ(false, canAssignFromInt(*CIC)); // const int *const
+
+  // Prove that dereference and assignment work.
+  EXPECT_EQ(1, *I);
+  EXPECT_EQ(1, *IC);
+  EXPECT_EQ(1, *CI);
+  EXPECT_EQ(1, *CIC);
+  *I = 2;
+  EXPECT_EQ(2, Number);
+  *IC = 3;
+  EXPECT_EQ(3, Number);
+
+  // Construct some proxy iterators and check whether they can be assigned to.
+  IntProxyIterator P(&Number);
+  const IntProxyIterator PC(&Number);
+  ConstIntProxyIterator CP(&Number);
+  const ConstIntProxyIterator CPC(&Number);
+  EXPECT_EQ(true, canAssignFromInt(*P));    // int *
+  EXPECT_EQ(true, canAssignFromInt(*PC));   // int *const
+  EXPECT_EQ(false, canAssignFromInt(*CP));  // const int *
+  EXPECT_EQ(false, canAssignFromInt(*CPC)); // const int *const
+
+  // Prove that dereference and assignment work.
+  EXPECT_EQ(3, (*P).I);
+  EXPECT_EQ(3, (*PC).I);
+  EXPECT_EQ(3, (*CP).I);
+  EXPECT_EQ(3, (*CPC).I);
+  *P = 4;
+  EXPECT_EQ(4, Number);
+  *PC = 5;
+  EXPECT_EQ(5, Number);
+}
+
 // pointeE_iterator
 static_assert(IsAdaptedIterCategorySame<pointee_iterator_defaulted,
                                         RandomAccessIter>::value, "");
@@ -178,6 +262,16 @@ TEST(FilterIteratorTest, Lambda) {
   EXPECT_EQ((SmallVector<int, 3>{1, 3, 5}), Actual);
 }
 
+TEST(FilterIteratorTest, Enumerate) {
+  auto IsOdd = [](auto N) { return N.value() % 2 == 1; };
+  int A[] = {0, 1, 2, 3, 4, 5, 6};
+  auto Enumerate = llvm::enumerate(A);
+  SmallVector<int> Actual;
+  for (auto IndexedValue : make_filter_range(Enumerate, IsOdd))
+    Actual.push_back(IndexedValue.value());
+  EXPECT_EQ((SmallVector<int, 3>{1, 3, 5}), Actual);
+}
+
 TEST(FilterIteratorTest, CallableObject) {
   int Counter = 0;
   struct Callable {