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

[Markus Böck via Phabricator via llvm-commits]

zero9178 added a comment.

Implementation makes sense to me and matches my expectation. Wouldn't really know how to do this any simpler than the current state myself to be honest.



================
Comment at: llvm/include/llvm/ADT/STLExtras.h:29
 #include <algorithm>
+#include <bits/utility.h>
 #include <cassert>
----------------
Watch out, this is not a standard C++ header but an implementation detail from libstdc++


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2209
+
+  enumerator_result(const enumerator_result &) = default;
+  enumerator_result &operator=(const enumerator_result &) = default;
----------------
Any reason to explicitly default these? IIRC doing so even deletes the move constructors and move assignment


================
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 {
----------------
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.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2227
+      return value_reference_tuple(
+          get_values(std::index_sequence_for<Ranges...>{}));
+    }
----------------
I believe you meant this to be `range_reference_tuple` instead of `value_reference_tuple`, in which case you can just directly do `return get_values(...));`

Otherwise this 1) does not compile if calling `.value()` on the result with more than one range 2) would otherwise return a tuple with the value returned by `index()` in the first position.

Please add a test case for this case as well.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:2272
+  explicit enumerator_iter(result_iter Iter) : Result(Iter) {}
+  enumerator_iter(const enumerator_iter &) = default;
+  enumerator_iter &operator=(const enumerator_iter &) = default;
----------------
ditto with copy constructor here


================
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.
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144503