[PATCH] D144834: [ADT] Fix const-correctness issues in `zippy`

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Might've been nice to separate the adl_* support from the other work in this patch, but at least at a vague glance, it all looks good - thanks!



================
Comment at: llvm/unittests/ADT/IteratorTest.cpp:501-502
+  unsigned iters = 0;
+  for (auto [a, b, c] : zip_equal(SmallVector<int>{1, 2, 3}, std::string("abc"),
+                                  std::vector<bool>{true, false, true})) {
+    a = 3;
----------------
kuhar wrote:
> kuhar wrote:
> > dblaikie wrote:
> > > Does this code work? How? Wouldn't the lifetimes of the temporary ranges expire before the body of the loop runs?
> > These ranges get moved into the `tuple<...> storage;` inside `zippy`. From then on, we can use references obtained from this `storage` to access them. So this should not rely on any lifetime extensions on the temporaries passed to `zip_equal`.
> To confirm, I ran this under ASan and UBSan and they don't report anything once I fixed the definition of `MakeConst`
ah, yeah, MakeConst especially made me uncertain this was correct - but yeah, returning by value, and zip carrying its arguments by value when passed temporaries makes sense that it all works together now, thanks!

May be worth a comment on MakeConst about its return value being const intentionally (though it's obvious enough from the name I suppose) - since const on by-value return types is usually a mistake & I could imagine a linter or something trying to clean that up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144834