From e4e2520ac491eb7d13cde8aea87dd8668b622da0 Mon Sep 17 00:00:00 2001
From: Hannes Roth <hannesr@fb.com>
Date: Fri, 1 May 2015 09:22:05 -0700
Subject: [PATCH] (Wangle) Fix Executor problem

Summary:
None of these functions should be templated with `class Executor`.
Except `then(Executor, Args...)` because otherwise the compiler gets
confused. This was the combination that worked for both Clang and GCC,
don't ask me why. I'm assuming this puts it on a low priority...

I think this is also OK, because `setExecutor` takes an actual
`folly::Executor`, so even `then(Executor, Args...)` won't just work for any
`Executor`.

Test Plan: Run all the tests.

Reviewed By: jsedgwick@fb.com

Subscribers: folly-diffs@, jsedgwick, yfeldblum, chalfant

FB internal diff: D2036912

Tasks: 6838553

Signature: t1:2036912:1430493088:44f2ffe146298c3f978ac27a45b9b2e33b2b0422
---
 folly/futures/Future-inl.h     | 18 +++++++-----------
 folly/futures/Future.h         | 16 ++++++----------
 folly/futures/helpers.h        |  3 +--
 folly/futures/test/ViaTest.cpp | 13 +++++++------
 4 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 289429dc..b3ca83e6 100644
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -226,18 +226,17 @@ Future<T>::then(R(Caller::*func)(Args...), Caller *instance) {
   });
 }
 
-// TODO(6838553)
-#ifndef __clang__
 template <class T>
-template <class... Args>
-auto Future<T>::then(Executor* x, Args&&... args)
-  -> decltype(this->then(std::forward<Args>(args)...))
+template <class Executor, class Arg, class... Args>
+auto Future<T>::then(Executor* x, Arg&& arg, Args&&... args)
+  -> decltype(this->then(std::forward<Arg>(arg),
+                         std::forward<Args>(args)...))
 {
   auto oldX = getExecutor();
   setExecutor(x);
-  return this->then(std::forward<Args>(args)...).via(oldX);
+  return this->then(std::forward<Arg>(arg), std::forward<Args>(args)...).
+               via(oldX);
 }
-#endif
 
 template <class T>
 Future<void> Future<T>::then() {
@@ -424,7 +423,6 @@ Optional<Try<T>> Future<T>::poll() {
 }
 
 template <class T>
-template <typename Executor>
 inline Future<T> Future<T>::via(Executor* executor) && {
   throwIfInvalid();
 
@@ -434,7 +432,6 @@ inline Future<T> Future<T>::via(Executor* executor) && {
 }
 
 template <class T>
-template <typename Executor>
 inline Future<T> Future<T>::via(Executor* executor) & {
   throwIfInvalid();
 
@@ -530,8 +527,7 @@ inline Future<void> makeFuture(Try<void>&& t) {
 }
 
 // via
-template <typename Executor>
-Future<void> via(Executor* executor) {
+inline Future<void> via(Executor* executor) {
   return makeFuture().via(executor);
 }
 
diff --git a/folly/futures/Future.h b/folly/futures/Future.h
index 3acd7d46..924c3805 100644
--- a/folly/futures/Future.h
+++ b/folly/futures/Future.h
@@ -97,14 +97,12 @@ class Future {
   // The ref-qualifier allows for `this` to be moved out so we
   // don't get access-after-free situations in chaining.
   // https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/
-  template <typename Executor>
-  Future<T> via(Executor* executor) &&;
+  inline Future<T> via(Executor* executor) &&;
 
   /// This variant creates a new future, where the ref-qualifier && version
   /// moves `this` out. This one is less efficient but avoids confusing users
   /// when "return f.via(x);" fails.
-  template <typename Executor>
-  Future<T> via(Executor* executor) &;
+  inline Future<T> via(Executor* executor) &;
 
   /** True when the result (or exception) is ready. */
   bool isReady() const;
@@ -182,8 +180,6 @@ class Future {
   Future<typename isFuture<R>::Inner>
   then(R(Caller::*func)(Args...), Caller *instance);
 
-// TODO(6838553)
-#ifndef __clang__
   /// Execute the callback via the given Executor. The executor doesn't stick.
   ///
   /// Contrast
@@ -196,10 +192,10 @@ class Future {
   ///
   /// In the former both b and c execute via x. In the latter, only b executes
   /// via x, and c executes via the same executor (if any) that f had.
-  template <class... Args>
-  auto then(Executor* x, Args&&... args)
-    -> decltype(this->then(std::forward<Args>(args)...));
-#endif
+  template <class Executor, class Arg, class... Args>
+  auto then(Executor* x, Arg&& arg, Args&&... args)
+    -> decltype(this->then(std::forward<Arg>(arg),
+                           std::forward<Args>(args)...));
 
   /// Convenience method for ignoring the value and creating a Future<void>.
   /// Exceptions still propagate.
diff --git a/folly/futures/helpers.h b/folly/futures/helpers.h
index 97c418d1..744a01ac 100644
--- a/folly/futures/helpers.h
+++ b/folly/futures/helpers.h
@@ -131,8 +131,7 @@ Future<T> makeFuture(Try<T>&& t);
  *
  * @returns a void Future that will call back on the given executor
  */
-template <typename Executor>
-Future<void> via(Executor* executor);
+inline Future<void> via(Executor* executor);
 
 /** When all the input Futures complete, the returned Future will complete.
   Errors do not cause early termination; this Future will always succeed
diff --git a/folly/futures/test/ViaTest.cpp b/folly/futures/test/ViaTest.cpp
index 0675baf8..3ade6679 100644
--- a/folly/futures/test/ViaTest.cpp
+++ b/folly/futures/test/ViaTest.cpp
@@ -185,11 +185,9 @@ TEST(Via, chain3) {
   EXPECT_EQ(3, count);
 }
 
-// TODO(6838553)
-#ifndef __clang__
 TEST(Via, then2) {
   ManualExecutor x1, x2;
-  bool a,b,c;
+  bool a = false, b = false, c = false;
   via(&x1)
     .then([&]{ a = true; })
     .then(&x2, [&]{ b = true; })
@@ -212,8 +210,11 @@ TEST(Via, then2) {
 }
 
 TEST(Via, then2Variadic) {
-  struct Foo { void foo(Try<void>) {} };
+  struct Foo { bool a = false; void foo(Try<void>) { a = true; } };
   Foo f;
-  makeFuture().then(nullptr, &Foo::foo, &f);
+  ManualExecutor x;
+  makeFuture().then(&x, &Foo::foo, &f);
+  EXPECT_FALSE(f.a);
+  x.run();
+  EXPECT_TRUE(f.a);
 }
-#endif
-- 
2.34.1