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

Sun Dec 25 00:27:58 PST 2016

chandlerc added a comment.

Thanks for the review Dave and Eric! (Also, thanks for the help debugging and resolving issues on IRC Eric!)

I think everything is ship-shape, so I'm gonna land this and fight the build bots until it sticks. Shout if anything else catches your eye!

================
Comment at: include/llvm/ADT/STLExtras.h:455-456
+
+  std::tuple<IterTs...> iterators;
+  std::tuple<IterTs...> end_iterators;
+
----------------
dblaikie wrote:
> Any particular tradeoff between doing this representation compared to std::tuple<std::pair<IterTs>...> (where the pairs contain begin/end)?
> 
> Maybe I'll understand as I read further.
Nah, using a pair is much nicer. Half as many big tuple lookups.

I tried using iterator_range, but that's actually not a good fit because we're mutating things and doing other custom things.... But the pair seems nicer.

================
Comment at: include/llvm/ADT/STLExtras.h:460-461
+  //
+  // Returns true if it was able to increment the iterator. Returns false if
+  // the iterator is already at the end iterator.
+  template <size_t Index> bool incrementHelper() {
----------------
dblaikie wrote:
> should this be a \returns doxy comment to comment the return semantics more explicitly
I'm not such a big fan of that doxygen these days. It doesn't seem to support lots of things I want, such as the fact that there are two statements about the return here...

That said, if you see a good way to restructure the comment, by all means?

================
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>...};
+
----------------
EricWF wrote:
> EricWF wrote:
> > 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).
> After double checking the standard I'm confident to say this code has UB; and should be fixed before committing.
Fixed by initializing an array instead of just 'auto'. Required massaging C++ harder than I'd like, but such is life.

================
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?
Not really, because we can't move here as we need to access it twice. The reason for the forwarding reference is so that it can be either an L-value reference or an R-value reference pointing at a temporary iterator_range. We just need to look through it to the internal iterators.

================
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)...) {}
+
----------------
EricWF wrote:
> chandlerc wrote:
> > 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?
> > Not really, because we can't move here as we need to access it twice. The reason for the forwarding reference is so that it can be either an L-value reference or an R-value reference pointing at a temporary iterator_range. We just need to look through it to the internal iterators.
> 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.
Chatted with Eric on IRC about this - R-values are safe and intended. And we shouldn't forward when doing the begin thing for the reasons Eric lists.

Regarding ADL, I'm not convinced the complexity of doing it is worth the trouble particularly in an member init list. Anyways, we can add that if folks want it in the future.

================
Comment at: include/llvm/ADT/STLExtras.h:532-534
+  bool operator==(const concat_iterator &RHS) const {
+    return iterators == RHS.iterators;
+  }
----------------
dblaikie wrote:
> Usually suggested that op overloads that can be non-members should be non-members (could still be friend) to allow equal implicit conversions on both operands. Perhaps not applicable here (can't think of any particular implicit ops that'd be asymmetric here - but might be good as a matter of course/consistency/etc.
This is just how the current iterator_facade_base works. If we want to change that we can, but it shouldn't be here.

================
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.
I thought about this, and the static assert actually seemed more clear. You end up having to name the explicit template arguments and write a lot of boilerplate to handle them and error messages aren't clear *why* there are these explicit arguments. With the static assert it seemed more direct to me. Happy to consider follow-ups to go a different way though if there's a reason! =]

================
Comment at: include/llvm/IR/Module.h:595
+      global_object_iterator;
+  typedef concat_iterator<const GlobalObject, const_iterator,
+                          const_global_iterator>
----------------
EricWF wrote:
> You could break the line better using a `using` declaration instead. If only I knew @chandlerc's thoughts on manually editing whitespace...
If we want to start moving everything to using declarations, maybe... but not this patch please. =D

================
Comment at: unittests/ADT/STLExtrasTest.cpp:257-269
+
+TEST(STLExtrasTest, ConcatRange) {
+  std::vector<int> Expected = {1, 2, 3, 4, 5, 6, 7, 8};
+
+  std::vector<int> V1234 = {1, 2, 3, 4};
+  std::list<int> L56 = {5, 6};
+  SmallVector<int, 2> SV78 = {7, 8};
----------------
dblaikie wrote:
> dblaikie wrote:
> > What's the particular motivation for these data sets? (length 4, 2, 2 - vector, list, and SmallVector - seems like list's iterator's a strict subset of the others, so if it works for that, not sure adding the others adds lots of coverage & also not sure of the motivation for those lengths)
> > 
> > I guess there's no nice way to avoid creating a new data structure for the result? (I think GTest has some container comparison helpers, but I guess they don't work for arbitrary ranges?)
> Worth testing the copy/non-copy/lifetime-extension semantics when a temporary is passed in to concat?
The particular data sets were chosen at random, but contained meaningfully different iterator types and categories. Nothing more or less.

Regarding temporary tests, yep, added. Thanks for suggesting, they found a bug where i was missing a & in the declval.

================
Comment at: unittests/ADT/STLExtrasTest.cpp:266
+  std::vector<int> Test;
+  for (int &i : concat<int>(V1234, L56, SV78))
+    Test.push_back(i);
----------------
dblaikie wrote:
> Might be nice (could be a separate change) to skip the explicit value type parameter when the subranges have an exact match.
Yea, this does seem nice, but I'll leave it to a follow-up. I tried briefly and it added a lot of complexity. I probably just missed a good way to do it though.

https://reviews.llvm.org/D28093