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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:29
 #include <algorithm>
+#include <bits/utility.h>
 #include <cassert>
----------------
zero9178 wrote:
> Watch out, this is not a standard C++ header but an implementation detail from libstdc++
Gah, clangd end up pulling in funny includes sometimes...


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2224
+      using value_result = std::tuple_element_t<0, range_reference_tuple>;
+      return value_result(std::get<1>(*Iter));
+    } else {
----------------
zero9178 wrote:
> Am I correct that these casts here are only necassery since `zip` only ever uses the ranges const iterator? I don't see any other reasons since these are otherwise noop casts. 
> Fixing `zip` would also "fix" needing the `const` below in the test case on `Range`. Possibly out of scope for this patch.
Exactly. I've struggled with const references in my implementation, I'll try your suggestion to see if it simplifies things.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:168
+  // Check that we can modify the values when the iterator is a const
+  // reference. Note that 'Bool' is interesting because it's returned
+  // as a `std::vector` reference wrapper.
----------------
zero9178 wrote:
> Can you elaborate what you mean with const reference here? As far as I can tell, from all the types involved in the ranged for below none of them are const references. 
Thanks, the comment is out of sync with the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144503