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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: unittests/ADT/IteratorTest.cpp:331
 
+TEST(ZipIteratorTest, ZipLongestBasic) {
+  using namespace std;
----------------
I'd probably do this test the way you wrote the other test - two vectors of different lengths of interesting things, then a vector of pairs with the hardcoded pairs of optionals in it - then loop and compare each one? Rather than the bit fiddling, counting, etc?


================
Comment at: unittests/ADT/IteratorTest.cpp:429
 
+TEST(ZipIteratorTest, ZipLongestFilter) {
+  using namespace std;
----------------
I probably wouldn't include this test unless there's some really specific relationship between zip and filter - it's not practical to test all the combinations of iterators with each other, etc - so only the specific interface of each one should generally be tested, I think?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D48348