Update the llvm::enumerate utility class result_pair to use the 'iterator_traits<R>::reference' instead of 'ValueOfIter<R> &'.

[rriddle]

Summary

Update the llvm::enumerate helper class result_pair<R> to use the 'iterator_traits<R>::reference' type as the result of 'value()' instead 'ValueOfRange<R> &'. This enables support for iterators that return value types, i.e. non reference. This is a common pattern for some classes of iterators, e.g. mapped_iterator.

[mehdi_amini]

LGTM

[dblaikie]

My understanding was/is that iterators can't return values instead of references (that such a thing doesn't meet the standard requirements for iterators) and that iterators basically have to have internal storage for things like this.

I think that requirement comes from the requirement that if "(*i).j" is valid" for some iterator 'i' then "i->j" must also be valid - and since op-> returns a pointer, then there must be storage.

So perhaps mapped_iterator should be fixed to be conforming?

[rriddle]

My understanding is that input_iterators do not have to return a reference, and may return a value type or some other proxy object.

(Sorry if there is a specific website that should be linked)
See specification of LegacyInputIterator: https://en.cppreference.com/w/cpp/named_req/InputIterator
See example of iterator returning a value_type, std::istreambuf_iterator: https://en.cppreference.com/w/cpp/iterator/istreambuf_iterator

[dblaikie]

In D63632#1556532, @rriddle wrote:

My understanding is that input_iterators do not have to return a reference, and may return a value type or some other proxy object.

(Sorry if there is a specific website that should be linked)
See specification of LegacyInputIterator: https://en.cppreference.com/w/cpp/named_req/InputIterator

This shows the issue I was mentioning - the third row of the table, that "i->m" must be equivalent to (*i).m. streambuf_iterator sidesteps it a bit perhaps because it never has a type for which "->" is valid (well, maybe it can - with a user CharT with members).

It suggests it can be implemented in other ways - with an opaque handle type rather than an actual pointer to the element type - but that's not the issue with mapped_iterator, for instance. So it still seems like mapped_iterator can/should be fixed here?

See example of iterator returning a value_type, std::istreambuf_iterator: https://en.cppreference.com/w/cpp/iterator/istreambuf_iterator

[nlguillemot]

llvm/include/llvm/ADT/STLExtras.h

1525

I think this doesn't match the previous behavior.

I think const value_reference is like T & const, not like const T &.

I wish I could suggest using a typedef like const_value_reference = (...)::const_reference and returning that, but iterator_traits doesn't have such a member typedef.

Found this due to a compile error in some downstream code.

llvm/include/llvm/ADT/STLExtras.h

template <typename T> struct deref {
   }
 };
 
 namespace detail {
 
 template <typename R> class enumerator_iter;
 
 template <typename R> struct result_pair {
+  using value_reference =
+      typename std::iterator_traits<IterOfRange<R>>::reference;
+
   friend class enumerator_iter<R>;
 
   result_pair() = default;
   result_pair(std::size_t Index, IterOfRange<R> Iter)
       : Index(Index), Iter(Iter) {}
 
   result_pair<R> &operator=(const result_pair<R> &Other) {
     Index = Other.Index;
     Iter = Other.Iter;
     return *this;
   }
 
   std::size_t index() const { return Index; }
-  const ValueOfRange<R> &value() const { return *Iter; }
-  ValueOfRange<R> &value() { return *Iter; }
+  const value_reference value() const { return *Iter; }
+  value_reference value() { return *Iter; }
 
 private:
   std::size_t Index = std::numeric_limits<std::size_t>::max();
   IterOfRange<R> Iter;
 };
 
 template <typename R>
 class enumerator_iter