[PATCH] D23252: [ADT] Zip range adapter

[George Burgess IV via llvm-commits]

george.burgess.iv added a subscriber: george.burgess.iv.
george.burgess.iv added a comment.

Thanks for the patch!

Just a few drive-by 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...);
+}
+
----------------
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.)

================
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;
----------------
Is there a reason we can't just use `std::index_sequence`/`std::make_index_sequence`?

================
Comment at: include/llvm/ADT/STLExtras.h:317
@@ +316,3 @@
+
+// zip iterator for two or more iteratable types.
+template <typename T, typename U, typename... Args>
----------------
Please use `///` for function/class documentation.

================
Comment at: include/llvm/ADT/STLExtras.h:325
@@ +324,3 @@
+
+// zip iterator that, for the sake of efficiency, assumes the first iteratee to
+// be the shortest.
----------------
and here

================
Comment at: unittests/Support/IteratorTest.cpp:101
@@ -100,1 +100,3 @@
 
+TEST(ZipIteratorTest, Basic) {
+  using namespace std;
----------------
Can we have one or more tests for `zip_first`, please? :)


Repository:
  rL LLVM

https://reviews.llvm.org/D23252