[PATCH] D25100: Add STLExtras apply_tuple
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 30 14:22:00 PDT 2016
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
More information about the llvm-commits
mailing list