[PATCH] D145337: [ADT] Avoid needless iterator copies in `zippy`

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar marked 2 inline comments as done.
kuhar added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:789
+  template <size_t... Ns> void tup_inc(std::index_sequence<Ns...>) {
+    (void)(++std::get<Ns>(iterators), ...);
   }
----------------
zero9178 wrote:
> ultra nit: We only usually use explicit casts to void for silencing unused variable warning don't we? Looks a bit odd here imo
I added `(void)` to avoid potential warnings about unused return values, but thinking about it again, it's probably signal that shouldn't be ignored in the first place. Thanks!


================
Comment at: llvm/unittests/ADT/IteratorTest.cpp:731
+  // just when the range is moved into `zippy`.
+  EXPECT_GE(copy_count, 2u);
+  // We expect the number of copies to be much smaller than the number of loop
----------------
zero9178 wrote:
> How fragile is this test? Isn't the initial copy count more or less an implementation detail of zippy? Would it maybe suffice to create the zippy range before the loop, record the count, then do the loop, and then ensure that the count after the loop is equal to before?
The number of copies in the loop is still >0 (4 to be exact), but I followed your suggestion and separated the pre-loop count from the in-loop count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145337