[PATCH] D28093: [ADT] Add a generic concatenating iterator and range.

[Eric Fiselier via Phabricator via llvm-commits]

EricWF added inline comments.


================
Comment at: include/llvm/ADT/STLExtras.h:477
+    // Build a sequence of functions to increment each iterator if possible.
+    auto IncrementHelperFns = {&concat_iterator::incrementHelper<Ns>...};
+
----------------
If I'm not mistaken this deduces to `std::initializer_list<FuncType>`, and we shouldn't use `initializer_list` outside of performing initialization.

This should be changed to deduce as an array type instead (or any other sequenced container).


================
Comment at: include/llvm/ADT/STLExtras.h:521-522
+  template <typename... RangeTs>
+  explicit concat_iterator(RangeTs &&... Ranges)
+      : iterators(std::begin(Ranges)...), end_iterators(std::end(Ranges)...) {}
+
----------------
dblaikie wrote:
> If 'Ranges' is accepted by universal reference (or what's the preferred term these days?) should these begin/end call parameters be std::forward'd?
> & ADL begin/end call?
Should this accept Ranges as rvalues at all? It seems like it might hide a lifetime bug.

If we do want to accept rvalues I'm not convinced we should forward them. Doing so might result it `Range&&` binding to `begin(Range const&)` instead of `begin(Range&)`, and changing the constness of the returned iterator.

I agree we probably also want ADL here, but we'll need to bring in the generic array std::begin/std::end with using declarations.


================
Comment at: include/llvm/ADT/STLExtras.h:573-574
+detail::concat_range<ValueT, RangeTs...> concat(RangeTs &&... Ranges) {
+  static_assert(sizeof...(RangeTs) > 1,
+                "Need more than one range to concatenate!");
+  return detail::concat_range<ValueT, RangeTs...>(
----------------
dblaikie wrote:
> Would it be better to have a single explicit arg before the variadic template arg - rather than static_assert? I suppose it's much of a muchness.
You would need two explicit args because the third variadic pack can be empty.


================
Comment at: include/llvm/IR/Module.h:595
+      global_object_iterator;
+  typedef concat_iterator<const GlobalObject, const_iterator,
+                          const_global_iterator>
----------------
You could break the line better using a `using` declaration instead. If only I knew @chandlerc's thoughts on manually editing whitespace...


https://reviews.llvm.org/D28093