fix collect for non-default-constructible types, for real this time

Summary:
this was a fun one. Add a specialized implementation that builds up the results in a map with their indices and aggregates them into a vector at the end

Test Plan: unit tests

Reviewed By: hans@fb.com

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

FB internal diff: D2002444

Signature: t1:2002444:1429642589:ee5aa5e8c461db97a28642b9887b3158df317813

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index ea8368e8dee19d6bde59e9f066516e864afed4f3..5bc6f58d82226c3fae115d952000213387674cd5 100644 (file)
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -584,12 +584,44 @@ whenAll(InputIterator first, InputIterator last) {
 
 namespace detail {
 
+template <class, class, typename = void> struct CollectContextHelper;
+
+template <class T, class VecT>
+struct CollectContextHelper<T, VecT,
+    typename std::enable_if<std::is_same<T, VecT>::value>::type> {
+  static inline std::vector<T>& getResults(std::vector<VecT>& results) {
+    return results;
+  }
+};
+
+template <class T, class VecT>
+struct CollectContextHelper<T, VecT,
+    typename std::enable_if<!std::is_same<T, VecT>::value>::type> {
+  static inline std::vector<T> getResults(std::vector<VecT>& results) {
+    std::vector<T> finalResults;
+    finalResults.reserve(results.size());
+    for (auto& opt : results) {
+      finalResults.push_back(std::move(opt.value()));
+    }
+    return finalResults;
+  }
+};
+
 template <typename T>
 struct CollectContext {
-  explicit CollectContext(int n) : count(0), threw(false) {}
+
+  typedef typename std::conditional<
+    std::is_default_constructible<T>::value,
+    T,
+    Optional<T>
+   >::type VecT;
+
+  explicit CollectContext(int n) : count(0), threw(false) {
+    results.resize(n);
+  }
 
   Promise<std::vector<T>> p;
-  std::vector<T> results;
+  std::vector<VecT> results;
   std::atomic<size_t> count;
   std::atomic_bool threw;
 
@@ -600,7 +632,7 @@ struct CollectContext {
   }
 
   inline void setValue() {
-    p.setValue(std::move(results));
+    p.setValue(CollectContextHelper<T, VecT>::getResults(results));
   }
 
   inline void addResult(int i, Try<T>& t) {
@@ -610,7 +642,9 @@ struct CollectContext {
 
 template <>
 struct CollectContext<void> {
+
   explicit CollectContext(int n) : count(0), threw(false) {}
+
   Promise<void> p;
   std::atomic<size_t> count;
   std::atomic_bool threw;

diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp
index 8efa6c64baf8d91e829357944248e7eb0b4ad3af..edc9d3012b828775fa8b7d7d66a6773033e09bbf 100644 (file)
--- a/folly/futures/test/FutureTest.cpp
+++ b/folly/futures/test/FutureTest.cpp
@@ -879,6 +879,39 @@ TEST(Future, collect) {
   }
 }
 
+struct NotDefaultConstructible {
+  NotDefaultConstructible() = delete;
+  NotDefaultConstructible(int arg) : i(arg) {}
+  int i;
+};
+
+// We have a specialized implementation for non-default-constructible objects
+// Ensure that it works and preserves order
+TEST(Future, collectNotDefaultConstructible) {
+  vector<Promise<NotDefaultConstructible>> promises(10);
+  vector<Future<NotDefaultConstructible>> futures;
+  vector<int> indices(10);
+  std::iota(indices.begin(), indices.end(), 0);
+  random_shuffle(indices.begin(), indices.end());
+
+  for (auto& p : promises)
+    futures.push_back(p.getFuture());
+
+  auto allf = collect(futures.begin(), futures.end());
+
+  for (auto i : indices) {
+    EXPECT_FALSE(allf.isReady());
+    promises[i].setValue(NotDefaultConstructible(i));
+  }
+
+  EXPECT_TRUE(allf.isReady());
+  int i = 0;
+  for (auto val : allf.value()) {
+    EXPECT_EQ(i, val.i);
+    i++;
+  }
+}
+
 TEST(Future, whenAny) {
   {
     vector<Promise<int>> promises(10);