Page MenuHomePhabricator

Add STLExtras apply_tuple

Authored by zturner on Sep 30 2016, 9:22 AM.



This is based on the C++17 std::apply. Given a tuple std::tuple<Ts...> Tup and a Callbable F, this function invokes F(Ts...) and returns the result.

Diff Detail


Event Timeline

zturner updated this revision to Diff 73066.Sep 30 2016, 9:22 AM
zturner retitled this revision from to Add STLExtras apply_tuple.
zturner updated this object.
zturner added reviewers: mehdi_amini, dblaikie.
zturner added a subscriber: llvm-commits.
dblaikie accepted this revision.Sep 30 2016, 2:21 PM
dblaikie edited edge metadata.

Looks good - couple of simple/optional things.

714 ↗(On Diff #73066)

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)

91–114 ↗(On Diff #73066)

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));

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.

117–135 ↗(On Diff #73066)

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?

This revision is now accepted and ready to land.Sep 30 2016, 2:21 PM
pete added a subscriber: pete.Sep 30 2016, 2:30 PM

Whats the behavior here in terms of implicit conversions and overloading?

Its probably handled however std::forward already does such things, but is it worth documenting/testing just to be clear what the expectation is?

BTW, no need for this to block the LGTM from David. Just something for a potential future patch.

This revision was automatically updated to reflect the committed changes.