Use variant as payload for Result<T, E>

Use std::variant<std::monostate, T, std::unique_ptr<E>> as the payload
for Result allows T to be non-default-constructible and also moves some
bookkeeping from Result into std::variant.

Bug: dawn:2344
Change-Id: I3f7d1aa4b7776de0174052c4379e5abb1ef107c3
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/169361
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

src/dawn/common/Result.h

diff --git a/src/dawn/common/Result.h b/src/dawn/common/Result.h
index 7bcccfc..8000a0f 100644
--- a/src/dawn/common/Result.h
+++ b/src/dawn/common/Result.h
@@ -33,6 +33,7 @@
 #include <memory>
 #include <type_traits>
 #include <utility>
+#include <variant>
 
 #include "dawn/common/Assert.h"
 #include "dawn/common/Compiler.h"
@@ -224,7 +225,7 @@
 template <typename T, typename E>
 class [[nodiscard]] Result {
   public:
-    Result(T&& success);
+    Result(T success);
     Result(std::unique_ptr<E> error);
 
     Result(Result<T, E>&& other);
@@ -235,19 +236,11 @@
     bool IsError() const;
     bool IsSuccess() const;
 
-    T&& AcquireSuccess();
+    T AcquireSuccess();
     std::unique_ptr<E> AcquireError();
 
   private:
-    enum PayloadType {
-        Success = 0,
-        Error = 1,
-        Acquired = 2,
-    };
-    PayloadType mType;
-
-    std::unique_ptr<E> mError;
-    T mSuccess;
+    std::variant<std::monostate, T, std::unique_ptr<E>> mPayload;
 };
 
 // Implementation of Result<void, E>
@@ -478,52 +471,52 @@
 
 // Implementation of Result<T, E>
 template <typename T, typename E>
-Result<T, E>::Result(T&& success) : mType(Success), mSuccess(std::move(success)) {}
+Result<T, E>::Result(T success) : mPayload(std::move(success)) {}
 
 template <typename T, typename E>
-Result<T, E>::Result(std::unique_ptr<E> error) : mType(Error), mError(std::move(error)) {}
+Result<T, E>::Result(std::unique_ptr<E> error) : mPayload(std::move(error)) {}
 
 template <typename T, typename E>
 Result<T, E>::~Result() {
-    DAWN_ASSERT(mType == Acquired);
+    DAWN_ASSERT(std::holds_alternative<std::monostate>(mPayload));
 }
 
 template <typename T, typename E>
-Result<T, E>::Result(Result<T, E>&& other)
-    : mType(other.mType), mError(std::move(other.mError)), mSuccess(std::move(other.mSuccess)) {
-    other.mType = Acquired;
+Result<T, E>::Result(Result<T, E>&& other) {
+    *this = std::move(other);
 }
+
 template <typename T, typename E>
 Result<T, E>& Result<T, E>::operator=(Result<T, E>&& other) {
-    mType = other.mType;
-    mError = std::move(other.mError);
-    mSuccess = std::move(other.mSuccess);
-    other.mType = Acquired;
+    DAWN_ASSERT(std::holds_alternative<std::monostate>(mPayload));
+    std::swap(mPayload, other.mPayload);
     return *this;
 }
 
 template <typename T, typename E>
 bool Result<T, E>::IsError() const {
-    return mType == Error;
+    return std::holds_alternative<std::unique_ptr<E>>(mPayload);
 }
 
 template <typename T, typename E>
 bool Result<T, E>::IsSuccess() const {
-    return mType == Success;
+    return std::holds_alternative<T>(mPayload);
 }
 
 template <typename T, typename E>
-T&& Result<T, E>::AcquireSuccess() {
-    DAWN_ASSERT(mType == Success);
-    mType = Acquired;
-    return std::move(mSuccess);
+T Result<T, E>::AcquireSuccess() {
+    DAWN_ASSERT(IsSuccess());
+    auto payload = std::move(mPayload);
+    mPayload = {};
+    return std::move(std::get<T>(payload));
 }
 
 template <typename T, typename E>
 std::unique_ptr<E> Result<T, E>::AcquireError() {
-    DAWN_ASSERT(mType == Error);
-    mType = Acquired;
-    return std::move(mError);
+    DAWN_ASSERT(IsError());
+    auto payload = std::move(mPayload);
+    mPayload = {};
+    return std::move(std::get<std::unique_ptr<E>>(payload));
 }
 
 }  // namespace dawn

src/dawn/tests/unittests/ResultTests.cpp

diff --git a/src/dawn/tests/unittests/ResultTests.cpp b/src/dawn/tests/unittests/ResultTests.cpp
index 8b5064d..44274e1 100644
--- a/src/dawn/tests/unittests/ResultTests.cpp
+++ b/src/dawn/tests/unittests/ResultTests.cpp
@@ -398,5 +398,58 @@
     TestSuccess(&result, {1.0f});
 }
 
+class NonDefaultConstructible {
+  public:
+    explicit NonDefaultConstructible(float v) : v(v) {}
+    bool operator==(const NonDefaultConstructible& other) const { return v == other.v; }
+    float v;
+};
+
+// Test constructing an error Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, ConstructingError) {
+    Result<NonDefaultConstructible, int> result(std::make_unique<int>(placeholderError));
+    TestError(&result, placeholderError);
+}
+
+// Test moving an error Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, MovingError) {
+    Result<NonDefaultConstructible, int> result(std::make_unique<int>(placeholderError));
+    Result<NonDefaultConstructible, int> movedResult(std::move(result));
+    TestError(&movedResult, placeholderError);
+}
+
+// Test returning an error Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, ReturningError) {
+    auto CreateError = []() -> Result<NonDefaultConstructible, int> {
+        return {std::make_unique<int>(placeholderError)};
+    };
+
+    Result<NonDefaultConstructible, int> result = CreateError();
+    TestError(&result, placeholderError);
+}
+
+// Test constructing a success Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, ConstructingSuccess) {
+    Result<NonDefaultConstructible, int> result(NonDefaultConstructible(1.0f));
+    TestSuccess(&result, NonDefaultConstructible(1.0f));
+}
+
+// Test moving a success Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, MovingSuccess) {
+    Result<NonDefaultConstructible, int> result(NonDefaultConstructible(1.0f));
+    Result<NonDefaultConstructible, int> movedResult(std::move(result));
+    TestSuccess(&movedResult, NonDefaultConstructible(1.0f));
+}
+
+// Test returning a success Result<T, E> for non-default-constructible T
+TEST(ResultNonDefaultConstructible, ReturningSuccess) {
+    auto CreateSuccess = []() -> Result<NonDefaultConstructible, int> {
+        return {NonDefaultConstructible(1.0f)};
+    };
+
+    Result<NonDefaultConstructible, int> result = CreateSuccess();
+    TestSuccess(&result, NonDefaultConstructible(1.0f));
+}
+
 }  // anonymous namespace
 }  // namespace dawn