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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Generally looks good - bunch of optional bits & pieces/thoughts.



================
Comment at: include/llvm/ADT/STLExtras.h:455-456
+
+  std::tuple<IterTs...> iterators;
+  std::tuple<IterTs...> end_iterators;
+
----------------
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.


================
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() {
----------------
should this be a \returns doxy comment to comment the return semantics more explicitly


================
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)...) {}
+
----------------
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?


================
Comment at: include/llvm/ADT/STLExtras.h:532-534
+  bool operator==(const concat_iterator &RHS) const {
+    return iterators == RHS.iterators;
+  }
----------------
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.


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


================
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};
----------------
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?)


================
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:
> 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?


================
Comment at: unittests/ADT/STLExtrasTest.cpp:266
+  std::vector<int> Test;
+  for (int &i : concat<int>(V1234, L56, SV78))
+    Test.push_back(i);
----------------
Might be nice (could be a separate change) to skip the explicit value type parameter when the subranges have an exact match.


https://reviews.llvm.org/D28093