[PATCH] D144503: [ADT] Allow `llvm::enumerate` to enumerate over multiple ranges

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D144503#4179141 <https://reviews.llvm.org/D144503#4179141>, @zero9178 wrote:

> In D144503#4179099 <https://reviews.llvm.org/D144503#4179099>, @kuhar wrote:
>
>> In D144503#4179026 <https://reviews.llvm.org/D144503#4179026>, @dblaikie wrote:
>>
>>> I think it's the InputIterator (ForwardIterators must be InputIterators) requirement is the one that makes value returns not possible in practice because they're required to support `i->m` as being equivalent to (*i).m`, which isn't possible, as far as I recall/understand without backing storage to return a pointer to?
>>>
>>> Oh, and ForwardIterator also says (on cppreference):
>>>
>>>   Let T be the value type of It. The type std::iterator_traits<It>::reference must be either
>>>     T& or T&& (since C++11) if It satisfies LegacyOutputIterator (It is mutable), or
>>>     const T& or const T&& (since C++11) otherwise (It is constant),
>>>   (where T is the type denoted by std::iterator_traits<It>::value_type)
>>
>> David and I talked offline and this interpretation of the standard is probably the correct one. To conform to the iterator requirements, we would have to store something like `std::optional<TupleOfReferences>` inside `zip_common` and materialize it on the first dereference. This would be a proper fix for all zip functions, but it doesn't seem like any part of llvm relies on `reference` being an actual reference right now, so it doesn't seem like a blocker.
>>
>> I will update all uses of `for (auto &it : enumerate(...))` in a separate revision before landing this and leave a `FIXME` for the reference type issues.
>
> I think its also fair to say that this is somewhat a historical mistake that was never properly followed (see `std::vector<bool>`) which is probably why this requirement was dropped in C++20 concepts for forward iterators: https://en.cppreference.com/w/cpp/iterator/forward_iterator

Yeah, sort of nice to see (insofar as I can see it/read the concepts as best I can) - op-> is awkward & weird when it's not supported, but probably the best tradeoff to lose that consistency for the greater flexibility. (Not sure what to make of comments like "Pointers and references obtained from a forward iterator into a range remain valid while the range exists.".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144503