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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D144503#4145819 <https://reviews.llvm.org/D144503#4145819>, @kuhar wrote:

> In D144503#4145802 <https://reviews.llvm.org/D144503#4145802>, @dblaikie wrote:
>
>> The total amount of nesting in the call? 3, in this case - flatten+enumerate+zip - I wouldn't be against having a wrapper, I guess, that does those things together - while still having tehm implemented as independent features, rather than having enumerate or zip being aware of each other.
>
> Your comments got me thinking about a different solution; I'd like to try to decompose this as follows something like this:
>
> - use the `zippy` to take care of enumeration of all subranges, including the index. Drop all the custom iterator/enumerator types etc.
> - add a wrapper reference type that produces a nice interface over each tuple of elements from `zippy` (very similar to `enumerator_result` here)
> - use something like `map_range` to expose the zipped ranges via this wrapper element reference type
>
> In pseudocode:
>
>   auto enumerate(ranges...) {
>     return map_range(zippy<zip_second>(index_stream{}, ranges...), to_enumerate_result);
>   }
>
> WDYT?
>
>> Not sure I follow - the generic code would be using `flatten(enumerate(zip...))` so it'd still always be flattening the enumerate+zip, if the generic code were calling flatten+enumerate, without knowing the contents of the enumerate - it'd be asking to flatten whatever was being enumerated. I don't follow there being an unintentional ambiguity between the two.
>
> I was worried about code that takes a variadic number of ranges and passes them to enumerate, but you are right that this is a non-issue if we leave enumerate unary and have a new wrapper function.
>
> So a potential solution would be to introduce `enumerate_zip` that takes 2 or more ranges and is implemented like this:
>
>   auto enumerate_zip(first, second, rest...) {
>     return zippy<zip_second>(index_stream{}, first, second, rest...);
>   } 
>
> and does not support `.index()` and `.value()`. I think that's also a viable option and doesn't even need `flatten` if we give up on those two accessors anyway.

Yeah, either of these sound OK to me - thanks for working through them.

(be nice if enumerate_resulrt or equivalent were a bit simpler, but I haven't looked closely at all the complexities/understood their motivation)

I guess `llvm::enumerate` doesn't have any particular API it's trying to emulate (like I don't immediately see a standardized version on the horizon? Is there a boost or other one that we should be trying to keep this looking similar to)? If there is some similar one we're trying to stay close to, does that one support N-ary enumerate? if it doesn't, then I'd be happy to keep the current enumerate as singular, and add the separate enumerate_zip, probably. As much as the "wrapping it in a nice result with `index()` and `value()`" is a bit of a pain/bunch of work compared to the generic tuple, I guess having the index named is worthwhile - even with the prevalence of structured bindings, so I don't object to the `enumerate_result` or equivalent being implemented)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144503