[PATCH] D23252: [ADT] Zip range adapter

[bryant via llvm-commits]

bryant 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...);
+}
+
----------------
george.burgess.iv wrote:
> dblaikie wrote:
> > 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)
> I think `constexpr` makes some of the compilers we support unhappy; please use `LLVM_CONSTEXPR` instead.
> 
> (Also, FWIW,
> ```
> template <typename... Bs> bool all_true(Bs... bools) {
>   return all_of({bool(bools)...}, identity<bool>{});
> }
> ```
> would eliminate the potential recursion. Looks like this `zip` magic is declared above `llvm::all_of`, though.)
this is good. i'm going to steal this, if you don't mind.

================
Comment at: include/llvm/ADT/STLExtras.h:245
@@ +244,3 @@
+
+template <unsigned N, unsigned... Ns> struct NatList {
+  using eval = typename NatList<N - 1, N - 1, Ns...>::eval;
----------------
george.burgess.iv wrote:
> Is there a reason we can't just use `std::index_sequence`/`std::make_index_sequence`?
according to http://en.cppreference.com/w/cpp/utility/integer_sequence it's c++14, whereas i'm aiming for c++11.


Repository:
  rL LLVM

https://reviews.llvm.org/D23252