(Wangle) whenAll should take rvalue references instead of references

Summary: Don't need to keep the old futures around.

Test Plan: `FutureTest.cpp`

Reviewed By: hans@fb.com

FB internal diff: D1197360

diff --git a/folly/wangle/Future-inl.h b/folly/wangle/Future-inl.h
index 46a63d525cfcff4d0cf736b89b40219387626590..9a606915f85ffc1fd19bc911f194127549e24029 100644 (file)
--- a/folly/wangle/Future-inl.h
+++ b/folly/wangle/Future-inl.h
@@ -283,12 +283,12 @@ makeFuture(E const& e) {
 
 template <typename... Fs>
 typename detail::VariadicContext<typename Fs::value_type...>::type
-whenAll(Fs&... fs)
+whenAll(Fs&&... fs)
 {
   auto ctx = new detail::VariadicContext<typename Fs::value_type...>();
   ctx->total = sizeof...(fs);
   auto f_saved = ctx->p.getFuture();
-  detail::whenAllVariadicHelper(ctx, fs...);
+  detail::whenAllVariadicHelper(ctx, std::forward<Fs>(fs)...);
   return std::move(f_saved);
 }
 

diff --git a/folly/wangle/Future.h b/folly/wangle/Future.h
index 3b3bcf0698096abb1af149c362da65d9e8e4ac12..f590e0915dd7f7daa092ac35ba41bad1534aa417 100644 (file)
--- a/folly/wangle/Future.h
+++ b/folly/wangle/Future.h
@@ -197,10 +197,9 @@ whenAll(InputIterator first, InputIterator last);
 /// This version takes a varying number of Futures instead of an iterator.
 /// The return type for (Future<T1>, Future<T2>, ...) input
 /// is a Future<std::tuple<Try<T1>, Try<T2>, ...>>.
-// XXX why does it take Fs& instead of Fs&&?
 template <typename... Fs>
 typename detail::VariadicContext<typename Fs::value_type...>::type
-whenAll(Fs&... fs);
+whenAll(Fs&&... fs);
 
 /** The result is a pair of the index of the first Future to complete and
   the Try. If multiple Futures complete at the same time (or are already

diff --git a/folly/wangle/detail.h b/folly/wangle/detail.h
index f939cb0db556803de951a04fc4de71497cc083d1..b1d6ade42a2b60dfbe0f4543512622a3bef6a6c0 100644 (file)
--- a/folly/wangle/detail.h
+++ b/folly/wangle/detail.h
@@ -104,10 +104,9 @@ struct VariadicContext {
 
 template <typename... Ts, typename THead, typename... Fs>
 typename std::enable_if<sizeof...(Fs) == 0, void>::type
-whenAllVariadicHelper(VariadicContext<Ts...> *ctx, THead& head, Fs&... tail) {
+whenAllVariadicHelper(VariadicContext<Ts...> *ctx, THead&& head, Fs&&... tail) {
   head.setContinuation([ctx](Try<typename THead::value_type>&& t) {
-    const size_t i = sizeof...(Ts) - sizeof...(Fs) - 1;
-    std::get<i>(ctx->results) = std::move(t);
+    std::get<sizeof...(Ts) - sizeof...(Fs) - 1>(ctx->results) = std::move(t);
     if (++ctx->count == ctx->total) {
       ctx->p.setValue(std::move(ctx->results));
       delete ctx;
@@ -117,16 +116,16 @@ whenAllVariadicHelper(VariadicContext<Ts...> *ctx, THead& head, Fs&... tail) {
 
 template <typename... Ts, typename THead, typename... Fs>
 typename std::enable_if<sizeof...(Fs) != 0, void>::type
-whenAllVariadicHelper(VariadicContext<Ts...> *ctx, THead& head, Fs&... tail) {
+whenAllVariadicHelper(VariadicContext<Ts...> *ctx, THead&& head, Fs&&... tail) {
   head.setContinuation([ctx](Try<typename THead::value_type>&& t) {
-    const size_t i = sizeof...(Ts) - sizeof...(Fs) - 1;
-    std::get<i>(ctx->results) = std::move(t);
+    std::get<sizeof...(Ts) - sizeof...(Fs) - 1>(ctx->results) = std::move(t);
     if (++ctx->count == ctx->total) {
       ctx->p.setValue(std::move(ctx->results));
       delete ctx;
     }
   });
-  whenAllVariadicHelper(ctx, tail...); // recursive template tail call
+  // template tail-recursion
+  whenAllVariadicHelper(ctx, std::forward<Fs>(tail)...);
 }
 
 template <typename T>

diff --git a/folly/wangle/test/FutureTest.cpp b/folly/wangle/test/FutureTest.cpp
index 2468c580c0fb22229d4532a1ad0c68f57ee0b8e3..a70af1ab0af4b165e3467200da538fcaa1ac1f53 100644 (file)
--- a/folly/wangle/test/FutureTest.cpp
+++ b/folly/wangle/test/FutureTest.cpp
@@ -506,7 +506,7 @@ TEST(Future, whenAllVariadic) {
   Future<bool> fb = pb.getFuture();
   Future<int> fi = pi.getFuture();
   bool flag = false;
-  whenAll(fb, fi)
+  whenAll(std::move(fb), std::move(fi))
     .then([&](Try<std::tuple<Try<bool>, Try<int>>>&& t) {
       flag = true;
       EXPECT_TRUE(t.hasValue());