[PATCH] D25100: Add STLExtras apply_tuple

[David Blaikie via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - couple of simple/optional things.



> STLExtras.h:714
> +template <typename F, typename Tuple>
> +auto apply_tuple(F &&f, Tuple &&t) -> decltype(detail::apply_tuple_impl(
> +    std::forward<F>(f), std::forward<Tuple>(t),

Probably call it 'apply', the same as the standard one? I think that's generally what we're doing for other not-yet-standardized implementaitons (llvm::make_unique, etc)

> STLExtrasTest.cpp:91-114
> +  auto T = std::make_tuple(1, 2, 3, 4);
> +  auto U = llvm::apply_tuple(
> +      [](int A, int B, int C, int D) {
> +        return std::make_tuple(A - B, B - C, C - D, D - A);
> +      },
> +      T);
> +

I'd probably test slightly fewer cases (4 doesn't seem to be more interesting than 3, for example - I can see why 2 might be "degenerate"/boundary rather than a "middle of the road" test)

& might consider simplifying the testing to something like this:

  auto R = apply_tuple([](int A, const char* B, double C) {
    EXPECT_EQ(1, A);
    ...
    return 47;
  }, std::make_tuple(1, "foo", 3.14));
  EXPECT_EQ(47, R);

There's no reason the output should be related to the input by type or value - just hard code some things, test that the things passed in make it into the lambda, and the thing returned makes it out again.

> STLExtrasTest.cpp:117-135
> +struct deref_iterator_tuple {
> +  template <typename... Iters> auto operator()(Iters &&... Items) {
> +    return std::make_tuple(*Items...);
> +  }
> +};
> +
> +struct increment_iterator_tuple {

This seems to pull in a lot of code (all of std::vector, etc) to test what I would expect to be an isolated feature.

What cases are being exercised by this test case? Could it be a bit more targeted/explicit?

https://reviews.llvm.org/D25100