From d1c9cd049996c8a19a027045203880151413cf23 Mon Sep 17 00:00:00 2001
From: Tom Cherry <tomcherry@google.com>
Date: Tue, 12 Sep 2017 14:44:56 -0700
Subject: [PATCH] init: fix hiding of move constructors of Result<T>

This is needed to have Result<Result<T>> work correctly.

Test: init unit tests
Change-Id: If7d23d1ea13f3727b567d3baf0eee1d8d0e5a196
---
 init/result.h        | 10 ++++++++--
 init/result_test.cpp | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/init/result.h b/init/result.h
index 36c3b8324..fc0396245 100644
--- a/init/result.h
+++ b/init/result.h
@@ -153,8 +153,14 @@ inline Error ErrnoError() {
 template <typename T>
 class Result {
   public:
-    template <typename... U>
-    Result(U&&... result) : contents_(std::in_place_index_t<0>(), std::forward<U>(result)...) {}
+    Result() {}
+
+    template <typename U, typename... V,
+              typename = std::enable_if_t<!(std::is_same_v<std::decay_t<U>, Result<T>> &&
+                                            sizeof...(V) == 0)>>
+    Result(U&& result, V&&... results)
+        : contents_(std::in_place_index_t<0>(), std::forward<U>(result),
+                    std::forward<V>(results)...) {}
 
     Result(Error&& error) : contents_(std::in_place_index_t<1>(), error.str(), error.get_errno()) {}
     Result(const ResultError& result_error)
diff --git a/init/result_test.cpp b/init/result_test.cpp
index 19caaf5b5..327b4444f 100644
--- a/init/result_test.cpp
+++ b/init/result_test.cpp
@@ -276,6 +276,49 @@ TEST(result, no_copy_on_return) {
     EXPECT_EQ(0U, ConstructorTracker::move_assignment_called);
 }
 
+// Below two tests require that we do not hide the move constructor with our forwarding reference
+// constructor.  This is done with by disabling the forwarding reference constructor if its first
+// and only type is Result<T>.
+TEST(result, result_result_with_success) {
+    auto return_result_result_with_success = []() -> Result<Result<Success>> {
+        return Result<Success>();
+    };
+    auto result = return_result_result_with_success();
+    ASSERT_TRUE(result);
+    ASSERT_TRUE(*result);
+
+    auto inner_result = result.value();
+    ASSERT_TRUE(inner_result);
+}
+
+TEST(result, result_result_with_failure) {
+    auto return_result_result_with_error = []() -> Result<Result<Success>> {
+        return Result<Success>(ResultError("failure string", 6));
+    };
+    auto result = return_result_result_with_error();
+    ASSERT_TRUE(result);
+    ASSERT_FALSE(*result);
+    EXPECT_EQ("failure string", result->error_string());
+    EXPECT_EQ(6, result->error_errno());
+}
+
+// This test requires that we disable the forwarding reference constructor if Result<T> is the
+// *only* type that we are forwarding.  In otherwords, if we are forwarding Result<T>, int to
+// construct a Result<T>, then we still need the constructor.
+TEST(result, result_two_parameter_constructor_same_type) {
+    struct TestStruct {
+        TestStruct(int value) : value_(value) {}
+        TestStruct(Result<TestStruct> result, int value) : value_(result->value_ * value) {}
+        int value_;
+    };
+
+    auto return_test_struct = []() -> Result<TestStruct> { return {Result<TestStruct>(6), 6}; };
+
+    auto result = return_test_struct();
+    ASSERT_TRUE(result);
+    EXPECT_EQ(36, result->value_);
+}
+
 TEST(result, die_on_access_failed_result) {
     Result<std::string> result = Error();
     ASSERT_DEATH(*result, "");