[PATCH] D48348: [ADT] Add zip_longest iterators.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D48348#1315209 <https://reviews.llvm.org/D48348#1315209>, @Meinersbur wrote:

> Thanks for accepting. I expected a discussion on whether zip_longest_default or zip_longest_optional is better (or make llvm::Optional support references and use zip_longest_optional with `llvm::Optional<T&>`). Should I commit both, as-is?


Ah, sure - I'd probably prefer Optional over default (because the Optional makes it strictly more descriptive/non-lossy) & just call it zip_longest. We could have another iterator adapter that collapses optionals to default constructed instances in tuples, for instance... though I guess that'd be a bit single-use anyway.



================
Comment at: unittests/ADT/IteratorTest.cpp:464-479
+  using namespace std;
+  vector<unsigned> pi{3, 1, 4, 1, 5, 9};
+
+  int iters = 0;
+  auto zipped = zip_longest_optional(pi, vector<bool>{1, 1, 0, 1});
+  for (auto tup : make_filter_range(zipped, [](decltype(zipped)::value_type t) {
+         return !get<1>(t).hasValue() || get<1>(t).getValue();
----------------
This testing is a bit convoluted - the bit testing, etc. Could it be done in a more direct manner, perhaps? (either stamping out/loop unrolling each value & testing the values directly against known constants (rather than against each other) or with a separate container containing the expected resulting pairs - and comparing those in a loop?)

(similarly below)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D48348