From d6cc1fe099d492b0dfb335190fab9785d1a1a4c9 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Wed, 17 Jul 2019 19:01:50 +0000 Subject: [PATCH] Add an implementation of Result The existing implementation of Result with tagged pointers was not able to handle constant pointers for the result. This is required in follow-up CLs to return internal formats in a ResultOrError. This CL extracts the tagged pointer logic out of Result so it can be shared with Result. Tests are also added to cover Result. BUG=dawn:128 Change-Id: Id19ae8e1153bcfcaf94d95ac314faf2b23af6f91 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/9100 Commit-Queue: Kai Ninomiya Reviewed-by: Austin Eng Reviewed-by: Kai Ninomiya --- src/common/BUILD.gn | 1 + src/common/Result.cpp | 30 ++++++ src/common/Result.h | 152 +++++++++++++++++++++------- src/tests/unittests/ResultTests.cpp | 45 ++++++++ 4 files changed, 191 insertions(+), 37 deletions(-) create mode 100644 src/common/Result.cpp diff --git a/src/common/BUILD.gn b/src/common/BUILD.gn index 71ecbe7ff3..0d05c275b3 100644 --- a/src/common/BUILD.gn +++ b/src/common/BUILD.gn @@ -86,6 +86,7 @@ if (is_win || is_linux || is_mac) { "Math.cpp", "Math.h", "Platform.h", + "Result.cpp", "Result.h", "Serial.h", "SerialMap.h", diff --git a/src/common/Result.cpp b/src/common/Result.cpp new file mode 100644 index 0000000000..a4132cd0be --- /dev/null +++ b/src/common/Result.cpp @@ -0,0 +1,30 @@ +// Copyright 2019 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "common/Result.h" + +// Implementation details of the tagged pointer Results +namespace detail { + + intptr_t MakePayload(const void* pointer, PayloadType type) { + intptr_t payload = reinterpret_cast(pointer); + ASSERT((payload & 3) == 0); + return payload | type; + } + + PayloadType GetPayloadType(intptr_t payload) { + return static_cast(payload & 3); + } + +} // namespace detail diff --git a/src/common/Result.h b/src/common/Result.h index b4824fe62f..cd63a7a848 100644 --- a/src/common/Result.h +++ b/src/common/Result.h @@ -85,6 +85,27 @@ constexpr size_t alignof_if_defined_else_default + static T* GetSuccessFromPayload(intptr_t payload); + template + static E* GetErrorFromPayload(intptr_t payload); + + constexpr static intptr_t kEmptyPayload = Empty; +} // namespace detail + template class DAWN_NO_DISCARD Result { public: @@ -108,21 +129,33 @@ class DAWN_NO_DISCARD Result { E* AcquireError(); private: - enum PayloadType { - Success = 0, - Error = 1, - Empty = 2, - }; + intptr_t mPayload = detail::kEmptyPayload; +}; - // Utility functions to manipulate the tagged pointer. Some of them don't need to be templated - // but we really want them inlined so we keep them in the headers - static intptr_t MakePayload(void* pointer, PayloadType type); - static PayloadType GetPayloadType(intptr_t payload); - static T* GetSuccessFromPayload(intptr_t payload); - static E* GetErrorFromPayload(intptr_t payload); +template +class DAWN_NO_DISCARD Result { + public: + static_assert(alignof_if_defined_else_default >= 4, + "Result reserves two bits for tagging pointers"); + static_assert(alignof_if_defined_else_default >= 4, + "Result reserves two bits for tagging pointers"); - constexpr static intptr_t kEmptyPayload = Empty; - intptr_t mPayload = kEmptyPayload; + Result(const T* success); + Result(E* error); + + Result(Result&& other); + Result& operator=(Result&& other); + + ~Result(); + + bool IsError() const; + bool IsSuccess() const; + + const T* AcquireSuccess(); + E* AcquireError(); + + private: + intptr_t mPayload = detail::kEmptyPayload; }; // Catchall definition of Result implemented as a tagged struct. It could be improved to use @@ -205,79 +238,124 @@ E* Result::AcquireError() { return error; } +// Implementation details of the tagged pointer Results +namespace detail { + + template + T* GetSuccessFromPayload(intptr_t payload) { + ASSERT(GetPayloadType(payload) == Success); + return reinterpret_cast(payload); + } + + template + E* GetErrorFromPayload(intptr_t payload) { + ASSERT(GetPayloadType(payload) == Error); + return reinterpret_cast(payload ^ 1); + } + +} // namespace detail + // Implementation of Result template -Result::Result(T* success) : mPayload(MakePayload(success, Success)) { +Result::Result(T* success) : mPayload(detail::MakePayload(success, detail::Success)) { } template -Result::Result(E* error) : mPayload(MakePayload(error, Error)) { +Result::Result(E* error) : mPayload(detail::MakePayload(error, detail::Error)) { } template Result::Result(Result&& other) : mPayload(other.mPayload) { - other.mPayload = kEmptyPayload; + other.mPayload = detail::kEmptyPayload; } template Result& Result::operator=(Result&& other) { - ASSERT(mPayload == kEmptyPayload); + ASSERT(mPayload == detail::kEmptyPayload); mPayload = other.mPayload; - other.mPayload = kEmptyPayload; + other.mPayload = detail::kEmptyPayload; return *this; } template Result::~Result() { - ASSERT(mPayload == kEmptyPayload); + ASSERT(mPayload == detail::kEmptyPayload); } template bool Result::IsError() const { - return GetPayloadType(mPayload) == Error; + return detail::GetPayloadType(mPayload) == detail::Error; } template bool Result::IsSuccess() const { - return GetPayloadType(mPayload) == Success; + return detail::GetPayloadType(mPayload) == detail::Success; } template T* Result::AcquireSuccess() { - T* success = GetSuccessFromPayload(mPayload); - mPayload = kEmptyPayload; + T* success = detail::GetSuccessFromPayload(mPayload); + mPayload = detail::kEmptyPayload; return success; } template E* Result::AcquireError() { - E* error = GetErrorFromPayload(mPayload); - mPayload = kEmptyPayload; + E* error = detail::GetErrorFromPayload(mPayload); + mPayload = detail::kEmptyPayload; return error; } +// Implementation of Result template -intptr_t Result::MakePayload(void* pointer, PayloadType type) { - intptr_t payload = reinterpret_cast(pointer); - ASSERT((payload & 3) == 0); - return payload | type; +Result::Result(const T* success) + : mPayload(detail::MakePayload(success, detail::Success)) { } template -typename Result::PayloadType Result::GetPayloadType(intptr_t payload) { - return static_cast(payload & 3); +Result::Result(E* error) : mPayload(detail::MakePayload(error, detail::Error)) { } template -T* Result::GetSuccessFromPayload(intptr_t payload) { - ASSERT(GetPayloadType(payload) == Success); - return reinterpret_cast(payload); +Result::Result(Result&& other) : mPayload(other.mPayload) { + other.mPayload = detail::kEmptyPayload; } template -E* Result::GetErrorFromPayload(intptr_t payload) { - ASSERT(GetPayloadType(payload) == Error); - return reinterpret_cast(payload ^ 1); +Result& Result::operator=(Result&& other) { + ASSERT(mPayload == detail::kEmptyPayload); + mPayload = other.mPayload; + other.mPayload = detail::kEmptyPayload; + return *this; +} + +template +Result::~Result() { + ASSERT(mPayload == detail::kEmptyPayload); +} + +template +bool Result::IsError() const { + return detail::GetPayloadType(mPayload) == detail::Error; +} + +template +bool Result::IsSuccess() const { + return detail::GetPayloadType(mPayload) == detail::Success; +} + +template +const T* Result::AcquireSuccess() { + T* success = detail::GetSuccessFromPayload(mPayload); + mPayload = detail::kEmptyPayload; + return success; +} + +template +E* Result::AcquireError() { + E* error = detail::GetErrorFromPayload(mPayload); + mPayload = detail::kEmptyPayload; + return error; } // Implementation of Result diff --git a/src/tests/unittests/ResultTests.cpp b/src/tests/unittests/ResultTests.cpp index 46161418e8..e9e5e366c9 100644 --- a/src/tests/unittests/ResultTests.cpp +++ b/src/tests/unittests/ResultTests.cpp @@ -38,6 +38,7 @@ void TestSuccess(Result* result, T expectedSuccess) { static int dummyError = 0xbeef; static float dummySuccess = 42.0f; +static const float dummyConstSuccess = 42.0f; // Result @@ -138,6 +139,50 @@ TEST(ResultBothPointer, ReturningSuccess) { TestSuccess(&result, &dummySuccess); } +// Result + +// Test constructing an error Result +TEST(ResultBothPointerWithConstResult, ConstructingError) { + Result result(&dummyError); + TestError(&result, &dummyError); +} + +// Test moving an error Result +TEST(ResultBothPointerWithConstResult, MovingError) { + Result result(&dummyError); + Result movedResult(std::move(result)); + TestError(&movedResult, &dummyError); +} + +// Test returning an error Result +TEST(ResultBothPointerWithConstResult, ReturningError) { + auto CreateError = []() -> Result { return {&dummyError}; }; + + Result result = CreateError(); + TestError(&result, &dummyError); +} + +// Test constructing a success Result +TEST(ResultBothPointerWithConstResult, ConstructingSuccess) { + Result result(&dummyConstSuccess); + TestSuccess(&result, &dummyConstSuccess); +} + +// Test moving a success Result +TEST(ResultBothPointerWithConstResult, MovingSuccess) { + Result result(&dummyConstSuccess); + Result movedResult(std::move(result)); + TestSuccess(&movedResult, &dummyConstSuccess); +} + +// Test returning a success Result +TEST(ResultBothPointerWithConstResult, ReturningSuccess) { + auto CreateSuccess = []() -> Result { return {&dummyConstSuccess}; }; + + Result result = CreateSuccess(); + TestSuccess(&result, &dummyConstSuccess); +} + // Result // Test constructing an error Result