[PATCH] D23252: [ADT] Zip range adapter

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: include/llvm/ADT/STLExtras.h:241-243
@@ +240,5 @@
+
+template <typename B, typename... Bs> constexpr bool all_true(B b, Bs... rest) {
+  return b && all_true(rest...);
+}
+
----------------
bryant wrote:
> dblaikie wrote:
> > Recursive implementations only used in constexpr contexts are fine - but a recursive implementation used in a non-constexpr context may still be implemented recursively by the compiler & is usually best avoided. (this is a problem with constexpr currently - where the ideal implementation for constexpr is not the same as the ideal implementation for non-constexpr contexts)
> > 
> > & this isn't being called in a constexpr context, so I'm not sure of the value of flagging it as constexpr or designing it in a way that would be valid constexpr.
> can you find a case where `all_true` is codegened into a recursive call?
Can't say I've tried, no. Fair point, though - just not exactly idiomatic (& admittedly variadic templates are on the fringes since they're not often used so idiom is still certainly evolving). Maybe Richard'll have some opinion here too. (or other people - don't mean to limit the review in any way)

================
Comment at: include/llvm/ADT/STLExtras.h:301
@@ +300,3 @@
+  typedef typename NatList<sizeof...(Args)>::eval nat_list;
+  std::tuple<Args...> ts;
+
----------------
bryant wrote:
> dblaikie wrote:
> > This keeps a copy (or move) of all its arguments to allow for the rvalue case in a range-for loop? That would have surprising performance characteristics (implicit whole-container copies) for non-temporary parameters... 
> > 
> > I think that gets into some pretty major design questions about the behavior here - and the major design problem/question with range proposals in general.
> > 
> > We might need to have a broader discussion about the design for anything like this - might be worth moving that part of the conversation to llvm-dev as I expect it'll be a bit more involved. But maybe here's OK & we can rope a few people in.
> > 
> > 
> if by "non-temporary" you mean lvalue input, then no. the corresponding tuple member would be a reference. if it were a copy, the mutability demo in unit test wouldn't work. please correct me if i've misunderstood your question.
Ah, indeed. You've not misunderstood - thanks for pointing out the test/explaining.

If that's all it takes to solve the temporary/non-temporary issue of range adapters, I'm really happy to hear it. But given my lack of understanding here I'm going to punt to Richard Smith, Chandler Carruth, on the C++ committee & who've been, at least somewhat (more than me, at least) aware of the goings on of range proposals, etc.


Repository:
  rL LLVM

https://reviews.llvm.org/D23252