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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

I don't totally object - though, for myself (I think this was discussed previously, but I don't fully understand the refutation/disagreement) I think I'd prefer to expose the compositions themselves and probably not have a special return type, so the end result is:

  for (auto &[index, v1, v2] : zip(index{}, r1, r2))

And drop "enumerate" entirely? It doesn't seem like it adds enough - though I guess having a name for it is useful. *shrug*



================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2282-2283
+struct index_stream {
+  struct iterator : iterator_facade_base<iterator, std::forward_iterator_tag,
+                                         const iterator> {
+    std::size_t operator*() const { return Index; }
----------------
Could this be conforming/have a real reference type (& Return ref from op*)?


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2292
 
-  enumerator_iter<R> begin() {
-    return enumerator_iter<R>(0, adl_begin(TheRange));
-  }
-  enumerator_iter<R> begin() const {
-    return enumerator_iter<R>(0, adl_begin(TheRange));
-  }
+    bool operator==(const iterator &Other) const {
+      return Index == Other.Index;
----------------
prefer non-member (can be a friend) op overloads where possible - makes sure implicit conversions happen equally on LHS and RHS


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144503