[PATCH] D30853: Improve the genericity of `llvm::enumerate()`.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:894-898
+template<unsigned Size, typename R>
+SmallVector<detail::ValueOfRange<R>, Size>
+to_vector(R &&Range) {
+  return { std::begin(Range), std::end(Range) };
+}
----------------
Perhaps not relevant now - but eventually this might want to be generalized to arbitrary range conversion (conversion to a std::set? etc... )


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1040
+
+  bool operator==(const enumerator_iter<R> &RHS) const {
+    return Result.Iter == RHS.Result.Iter;
----------------
Usually op overloads should be non-members whenever possible (ensures implicit conversions happen symmetrically for left and right operands) - though perhaps that isn't possible with the iterator facade helper?


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1045
+  enumerator_iter<R> &operator=(const enumerator_iter<R> &Other) {
+    assert(&R == &Other.R);
+
----------------
This doesn't look valid - is it? (I think R is a type, not a variable, so shouldn't be valid in these expressions)


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1048
+    Result = Other.Result;
+  }
 
----------------
Missing return? This should've warned/broken the build/crashed-at-O0, etc? Was there some missing test coverage?


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1062
+  enumerator_iter<R> end() {
+    return enumerator_iter<R>(-1, std::end(TheRange));
+  }
----------------
If the index of the end is going to be -1, it could be used to quickly catch a few mistakes with iterators - assert(index != -1) in operator++, for example?


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1065-1067
+  template <unsigned N> SmallVector<result_pair<R>, N> toVector() const {
+    return SmallVector<result_pair<R>, N>(begin(), end());
+  }
----------------
Once there's a generic utility function for all ranges as you've provided with to_vector - is this member still useful/necessary? (indeed this function seems to be untested - the test case exercises the generic to_vector instead)


https://reviews.llvm.org/D30853