From b6c75110e63902ff4a92cb4c4ac1f83527210904 Mon Sep 17 00:00:00 2001
From: Oleh Stolyar <stolyar.oleh@gmail.com>
Date: Fri, 14 Dec 2018 12:35:40 +0000
Subject: [PATCH] Prefer T(...) over T{...} when they behave differently

...in gen::construct and gen::makeUnique. Also, add support
for aggregate initialization in gen::makeShared and tests for
all of them.
---
 include/rapidcheck/detail/ApplyTuple.h |  6 +--
 include/rapidcheck/gen/Build.hpp       | 58 +++++++++++++++++++++-----
 test/gen/BuildTests.cpp                | 43 +++++++++++++++++++
 test/util/Aggregate.h                  | 23 ++++++++++
 4 files changed, 116 insertions(+), 14 deletions(-)
 create mode 100644 test/util/Aggregate.h

diff --git a/include/rapidcheck/detail/ApplyTuple.h b/include/rapidcheck/detail/ApplyTuple.h
index a664c18f..079e7721 100644
--- a/include/rapidcheck/detail/ApplyTuple.h
+++ b/include/rapidcheck/detail/ApplyTuple.h
@@ -13,7 +13,7 @@ struct ApplyTupleImpl;
 
 template <typename... Ts, std::size_t... Indexes, typename Callable>
 struct ApplyTupleImpl<std::tuple<Ts...>, Callable, IndexSequence<Indexes...>> {
-  using ReturnType = typename std::result_of<Callable(Ts &&...)>::type;
+  using ReturnType = typename std::result_of<Callable&&(Ts &&...)>::type;
 
   static ReturnType apply(std::tuple<Ts...> &&tuple, Callable &&callable) {
     return callable(std::move(std::get<Indexes>(tuple))...);
@@ -24,7 +24,7 @@ template <typename... Ts, std::size_t... Indexes, typename Callable>
 struct ApplyTupleImpl<std::tuple<Ts...> &,
                       Callable,
                       IndexSequence<Indexes...>> {
-  using ReturnType = typename std::result_of<Callable(Ts &...)>::type;
+  using ReturnType = typename std::result_of<Callable&&(Ts &...)>::type;
 
   static ReturnType apply(std::tuple<Ts...> &tuple, Callable &&callable) {
     return callable(std::get<Indexes>(tuple)...);
@@ -35,7 +35,7 @@ template <typename... Ts, std::size_t... Indexes, typename Callable>
 struct ApplyTupleImpl<const std::tuple<Ts...> &,
                       Callable,
                       IndexSequence<Indexes...>> {
-  using ReturnType = typename std::result_of<Callable(const Ts &...)>::type;
+  using ReturnType = typename std::result_of<Callable&&(const Ts &...)>::type;
 
   static ReturnType apply(const std::tuple<Ts...> &tuple, Callable &&callable) {
     return callable(std::get<Indexes>(tuple)...);
diff --git a/include/rapidcheck/gen/Build.hpp b/include/rapidcheck/gen/Build.hpp
index 9436fd78..8b7b9f3d 100644
--- a/include/rapidcheck/gen/Build.hpp
+++ b/include/rapidcheck/gen/Build.hpp
@@ -1,5 +1,7 @@
 #pragma once
 
+#include <memory>
+
 #include "rapidcheck/gen/Tuple.h"
 #include "rapidcheck/detail/ApplyTuple.h"
 
@@ -119,6 +121,41 @@ class BuildMapper<T, rc::detail::IndexSequence<Indexes...>, Lenses...> {
   std::tuple<Lenses...> m_lenses;
 };
 
+template<typename T, typename... Args>
+typename std::enable_if<std::is_constructible<T, Args...>::value, T>::type
+construct(Args &&... args) {
+  return T(std::forward<Args>(args)...);
+}
+
+template<typename T, typename... Args>
+typename std::enable_if<!std::is_constructible<T, Args...>::value, T>::type
+construct(Args &&... args) {
+  return T{std::forward<Args>(args)...};
+}
+
+template<typename T, typename... Args>
+typename std::enable_if<std::is_constructible<T, Args...>::value, std::unique_ptr<T>>::type
+makeUnique(Args &&... args) {
+  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
+template<typename T, typename... Args>
+typename std::enable_if<!std::is_constructible<T, Args...>::value, std::unique_ptr<T>>::type
+makeUnique(Args &&... args) {
+  return std::unique_ptr<T>(new T{std::forward<Args>(args)...});
+}
+
+template<typename T, typename... Args>
+typename std::enable_if<std::is_constructible<T, Args...>::value, std::shared_ptr<T>>::type
+makeShared(Args &&... args) {
+  return std::make_shared<T>(std::forward<Args>(args)...);
+}
+
+template<typename T, typename... Args>
+typename std::enable_if<!std::is_constructible<T, Args...>::value, std::shared_ptr<T>>::type
+makeShared(Args &&... args) {
+  return std::shared_ptr<T>(new T{std::forward<Args>(args)...});
+}
 } // namespace detail
 
 template <typename T, typename... Args>
@@ -126,8 +163,9 @@ Gen<T> construct(Gen<Args>... gens) {
   return gen::map(gen::tuple(std::move(gens)...),
                   [](std::tuple<Args...> &&argsTuple) {
                     return rc::detail::applyTuple(
-                        std::move(argsTuple),
-                        [](Args &&... args) { return T{std::move(args)...}; });
+                      std::forward<std::tuple<Args...>>(argsTuple),
+                      detail::construct<T, Args...>
+                    );
                   });
 }
 
@@ -141,10 +179,9 @@ Gen<std::unique_ptr<T>> makeUnique(Gen<Args>... gens) {
   return gen::map(gen::tuple(std::move(gens)...),
                   [](std::tuple<Args...> &&argsTuple) {
                     return rc::detail::applyTuple(
-                        std::move(argsTuple),
-                        [](Args &&... args) {
-                          return std::unique_ptr<T>(new T{std::move(args)...});
-                        });
+                      std::forward<std::tuple<Args...>>(argsTuple),
+                      detail::makeUnique<T, Args...>
+                    );
                   });
 }
 
@@ -152,11 +189,10 @@ template <typename T, typename... Args>
 Gen<std::shared_ptr<T>> makeShared(Gen<Args>... gens) {
   return gen::map(gen::tuple(std::move(gens)...),
                   [](std::tuple<Args...> &&argsTuple) {
-                    return rc::detail::applyTuple(std::move(argsTuple),
-                                                  [](Args &&... args) {
-                                                    return std::make_shared<T>(
-                                                        std::move(args)...);
-                                                  });
+                    return rc::detail::applyTuple(
+                      std::forward<std::tuple<Args...>>(argsTuple),
+                      detail::makeShared<T, Args...>
+                    );
                   });
 }
 
diff --git a/test/gen/BuildTests.cpp b/test/gen/BuildTests.cpp
index e2e8a3c5..6629ba96 100644
--- a/test/gen/BuildTests.cpp
+++ b/test/gen/BuildTests.cpp
@@ -1,6 +1,7 @@
 #include <catch2/catch.hpp>
 #include <rapidcheck/catch.h>
 
+#include "util/Aggregate.h"
 #include "util/GenUtils.h"
 #include "util/Predictable.h"
 #include "util/Logger.h"
@@ -90,6 +91,20 @@ TEST_CASE("gen::construct") {
     RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
     RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
   }
+
+  SECTION("works with aggregate initializers") {
+    gen::construct<NestedAggregate>(gen::arbitrary<Aggregate>(),
+                                    gen::arbitrary<int>());
+    gen::construct<NestedAggregate>(gen::arbitrary<int>(),
+                                    gen::arbitrary<int>(),
+                                    gen::arbitrary<int>());
+  }
+
+  SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
+    auto a = gen::construct<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
+    std::vector<int> b(3, 3);
+    RC_ASSERT(a == b);
+  }
 }
 
 TEST_CASE("gen::makeUnique") {
@@ -122,6 +137,20 @@ TEST_CASE("gen::makeUnique") {
     RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
     RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
   }
+
+  SECTION("works with aggregate initializers") {
+    gen::makeUnique<NestedAggregate>(gen::arbitrary<Aggregate>(),
+                                     gen::arbitrary<int>());
+    gen::makeUnique<NestedAggregate>(gen::arbitrary<int>(),
+                                     gen::arbitrary<int>(),
+                                     gen::arbitrary<int>());
+  }
+
+  SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
+    auto a = gen::makeUnique<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
+    auto b = std::unique_ptr<std::vector<int>>(new std::vector<int>(3, 3));
+    RC_ASSERT(*a == *b);
+  }
 }
 
 TEST_CASE("gen::makeShared") {
@@ -154,6 +183,20 @@ TEST_CASE("gen::makeShared") {
     RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
     RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
   }
+
+  SECTION("works with aggregate initializers") {
+    gen::makeShared<NestedAggregate>(gen::arbitrary<Aggregate>(),
+                                     gen::arbitrary<int>());
+    gen::makeShared<NestedAggregate>(gen::arbitrary<int>(),
+                                     gen::arbitrary<int>(),
+                                     gen::arbitrary<int>());
+  }
+
+  SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
+    auto a = gen::makeShared<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
+    auto b = std::shared_ptr<std::vector<int>>(new std::vector<int>(3, 3));
+    RC_ASSERT(*a == *b);
+  }
 }
 
 namespace {
diff --git a/test/util/Aggregate.h b/test/util/Aggregate.h
new file mode 100644
index 00000000..75e8cf6c
--- /dev/null
+++ b/test/util/Aggregate.h
@@ -0,0 +1,23 @@
+#pragma once
+
+#include "rapidcheck.h"
+
+namespace rc {
+  struct Aggregate {
+    int x;
+    int y;
+  };
+
+  template<>
+  struct Arbitrary<Aggregate> {
+    static Gen<Aggregate> arbitrary() {
+      return gen::construct<Aggregate>(gen::arbitrary<int>(),
+                                       gen::arbitrary<int>());
+    }
+  };
+
+  struct NestedAggregate {
+    Aggregate a;
+    int z;
+  };
+} // namespace rc