From 8898db4486a307b2d4034446f25b87e25d40eccd Mon Sep 17 00:00:00 2001 From: Sven Over Date: Mon, 6 Feb 2017 08:13:01 -0800 Subject: [PATCH] execute callbacks as rvalue references Summary: Callable objects may implement a separate `operator()` for when they are called as rvalue references. For example, `folly::partial` will move captured objects to the function call when invoked as rvalue reference. That allows e.g. to capture pass a `unique_ptr` to `folly::partial` for a function that takes a `unique_ptr` by value or rvalue-reference. Callbacks for `folly::Future`s are only ever executed once. They may consume captured data. If the callback is an object that implements a `operator()() &&`, then that one should be invoked. This diff makes sure, callbacks passed to `folly::Future` are invoked as rvalue references. Reviewed By: ericniebler, fugalh Differential Revision: D4404186 fbshipit-source-id: 9f33e442a634acb8797183d3d869840d85bd5d15 --- folly/futures/Future-inl.h | 49 ++++++++++++++++--------------- folly/futures/test/FutureTest.cpp | 46 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h index cf3e3344..28a5ffaa 100644 --- a/folly/futures/Future-inl.h +++ b/folly/futures/Future-inl.h @@ -159,14 +159,16 @@ Future::thenImplementation(F&& func, detail::argResult) { in some circumstances, but I think it should be explicit not implicit in the destruction of the Future used to create it. */ - setCallback_([ funcm = std::forward(func), pm = std::move(p) ]( - Try && t) mutable { - if (!isTry && t.hasException()) { - pm.setException(std::move(t.exception())); - } else { - pm.setWith([&]() { return funcm(t.template get()...); }); - } - }); + setCallback_( + [ func = std::forward(func), pm = std::move(p) ](Try && t) mutable { + if (!isTry && t.hasException()) { + pm.setException(std::move(t.exception())); + } else { + pm.setWith([&]() { + return std::move(func)(t.template get()...); + }); + } + }); return f; } @@ -189,14 +191,14 @@ Future::thenImplementation(F&& func, detail::argResult) { auto f = p.getFuture(); f.core_->setExecutorNoLock(getExecutor()); - setCallback_([ funcm = std::forward(func), pm = std::move(p) ]( + setCallback_([ func = std::forward(func), pm = std::move(p) ]( Try && t) mutable { auto ew = [&] { if (!isTry && t.hasException()) { return std::move(t.exception()); } else { try { - auto f2 = funcm(t.template get()...); + auto f2 = std::move(func)(t.template get()...); // that didn't throw, now we can steal p f2.setCallback_([p = std::move(pm)](Try && b) mutable { p.setTry(std::move(b)); @@ -263,13 +265,14 @@ Future::onError(F&& func) { p.core_->setInterruptHandlerNoLock(core_->getInterruptHandler()); auto f = p.getFuture(); - setCallback_([ funcm = std::forward(func), pm = std::move(p) ]( - Try && t) mutable { - if (!t.template withException( - [&](Exn& e) { pm.setWith([&] { return funcm(e); }); })) { - pm.setTry(std::move(t)); - } - }); + setCallback_( + [ func = std::forward(func), pm = std::move(p) ](Try && t) mutable { + if (!t.template withException([&](Exn& e) { + pm.setWith([&] { return std::move(func)(e); }); + })) { + pm.setTry(std::move(t)); + } + }); return f; } @@ -290,12 +293,12 @@ Future::onError(F&& func) { Promise p; auto f = p.getFuture(); - setCallback_([ pm = std::move(p), funcm = std::forward(func) ]( + setCallback_([ pm = std::move(p), func = std::forward(func) ]( Try && t) mutable { if (!t.template withException([&](Exn& e) { auto ew = [&] { try { - auto f2 = funcm(e); + auto f2 = std::move(func)(e); f2.setCallback_([pm = std::move(pm)](Try && t2) mutable { pm.setTry(std::move(t2)); }); @@ -346,11 +349,11 @@ Future::onError(F&& func) { Promise p; auto f = p.getFuture(); setCallback_( - [ pm = std::move(p), funcm = std::forward(func) ](Try t) mutable { + [ pm = std::move(p), func = std::forward(func) ](Try t) mutable { if (t.hasException()) { auto ew = [&] { try { - auto f2 = funcm(std::move(t.exception())); + auto f2 = std::move(func)(std::move(t.exception())); f2.setCallback_([pm = std::move(pm)](Try t2) mutable { pm.setTry(std::move(t2)); }); @@ -387,9 +390,9 @@ Future::onError(F&& func) { Promise p; auto f = p.getFuture(); setCallback_( - [ pm = std::move(p), funcm = std::forward(func) ](Try t) mutable { + [ pm = std::move(p), func = std::forward(func) ](Try t) mutable { if (t.hasException()) { - pm.setWith([&] { return funcm(std::move(t.exception())); }); + pm.setWith([&] { return std::move(func)(std::move(t.exception())); }); } else { pm.setTry(std::move(t)); } diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp index f5adcf44..bad230bc 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -846,3 +846,49 @@ TEST(Future, RequestContext) { TEST(Future, makeFutureNoThrow) { makeFuture().value(); } + +TEST(Future, invokeCallbackReturningValueAsRvalue) { + struct Foo { + int operator()(int x) & { + return x + 1; + } + int operator()(int x) const& { + return x + 2; + } + int operator()(int x) && { + return x + 3; + } + }; + + Foo foo; + Foo const cfoo; + + // The callback will be copied when given as lvalue or const ref, and moved + // if provided as rvalue. Either way, it should be executed as rvalue. + EXPECT_EQ(103, makeFuture(100).then(foo).value()); + EXPECT_EQ(203, makeFuture(200).then(cfoo).value()); + EXPECT_EQ(303, makeFuture(300).then(Foo()).value()); +} + +TEST(Future, invokeCallbackReturningFutureAsRvalue) { + struct Foo { + Future operator()(int x) & { + return x + 1; + } + Future operator()(int x) const& { + return x + 2; + } + Future operator()(int x) && { + return x + 3; + } + }; + + Foo foo; + Foo const cfoo; + + // The callback will be copied when given as lvalue or const ref, and moved + // if provided as rvalue. Either way, it should be executed as rvalue. + EXPECT_EQ(103, makeFuture(100).then(foo).value()); + EXPECT_EQ(203, makeFuture(200).then(cfoo).value()); + EXPECT_EQ(303, makeFuture(300).then(Foo()).value()); +} -- 2.34.1