[PATCH] D112981: Fix iterator_adaptor_base/enumerator_iter to allow composition of llvm::enumerate with llvm::make_filter_range

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

dexonsmith added a comment.

This doesn't look correct to me.

Here are two examples that I think this patch breaks:

  // Setup: simple wrappers around `int *` and `const int *`.
  struct const_iterator : public iterator_adaptor_base<const_iterator, const int *> {};
  struct iterator : public iterator_adaptor_base<iterator, int *> {};
  
  // `const`-qualified iterators (wrapping `int *const` and `const int *const`).
  const int &ConstRef = *std::declval<const const_iterator &>(); // (example #1)
  int &Ref = *std::declval<const iterator &>(); // (example #2)

The `const`-qualification on `iterator_adaptor_base` should not affect the reference value:

- non-const iterator: `int *` (dereferencing returns `int &`)
- non-const iterator with `const`: `int *const` (dereferencing returns `int &`)
- const iterator: `const int *` (dereferencing returns `const int &`)
- const iterator with `const`: `const int *const` (dereferencing returns `const int &`)

I think I found the actual bug; see my inline comments.



================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1920-1921
 
   result_type &operator*() { return Result; }
   const result_type &operator*() const { return Result; }
 
----------------
This is the actual bug. The `const`-ness of the iterator shouldn't affect the `const`-ness of the returned value.

The non-const `operator*()` should be deleted and only a `const result_type&` should ever be returned. The caller should never be allowed to modify `Result.Iter` or `Result.Index`.


================
Comment at: llvm/include/llvm/ADT/iterator.h:299-300
 
-  ReferenceT operator*() const { return *I; }
+  const ReferenceT operator*() const { return *I; }
+  ReferenceT operator*() { return *I; }
 };
----------------
If you used `std::add_const_t<ReferenceT>` then I think (1) would start compiling again but (2) would still fail.

I think this code change would "fix" the problem you encountered:
```
lang=c++
ReferenceT operator*() const { return *const_cast<WrappedIteratorT &>(I); }
```
... but I think the actual bug is in the `WrappedIteratorT`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112981