From 8faccffbad255079f4c3de9db627b7bacceaceba Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Tue, 28 Apr 2020 06:12:31 -0700 Subject: [PATCH] Update StatusOr<> and tests Fixes some template issues that could lead to code not compiling when it otherwise should. PiperOrigin-RevId: 308809964 Change-Id: I9f2f9d4aff5f1a9cb967fb705a86fd7f49114f7a --- sandboxed_api/util/status_macros_test.cc | 30 +++++++++----------- sandboxed_api/util/status_matchers.h | 1 - sandboxed_api/util/statusor.h | 36 +++++++++++++----------- sandboxed_api/util/statusor_test.cc | 10 +++---- 4 files changed, 39 insertions(+), 38 deletions(-) diff --git a/sandboxed_api/util/status_macros_test.cc b/sandboxed_api/util/status_macros_test.cc index 1a7fa9e..f74c83c 100644 --- a/sandboxed_api/util/status_macros_test.cc +++ b/sandboxed_api/util/status_macros_test.cc @@ -19,6 +19,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/memory/memory.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "sandboxed_api/util/status.h" #include "sandboxed_api/util/status_matchers.h" @@ -31,8 +32,8 @@ TEST(ReturnIfError, ReturnsOnErrorStatus) { auto func = []() -> absl::Status { SAPI_RETURN_IF_ERROR(absl::OkStatus()); SAPI_RETURN_IF_ERROR(absl::OkStatus()); - SAPI_RETURN_IF_ERROR(absl::Status(absl::StatusCode::kUnknown, "EXPECTED")); - return absl::Status(absl::StatusCode::kUnknown, "ERROR"); + SAPI_RETURN_IF_ERROR(absl::UnknownError("EXPECTED")); + return absl::UnknownError("ERROR"); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); @@ -41,9 +42,8 @@ TEST(ReturnIfError, ReturnsOnErrorStatus) { TEST(ReturnIfError, ReturnsOnErrorFromLambda) { auto func = []() -> absl::Status { SAPI_RETURN_IF_ERROR([] { return absl::OkStatus(); }()); - SAPI_RETURN_IF_ERROR( - [] { return absl::Status(absl::StatusCode::kUnknown, "EXPECTED"); }()); - return absl::Status(absl::StatusCode::kUnknown, "ERROR"); + SAPI_RETURN_IF_ERROR([] { return absl::UnknownError("EXPECTED"); }()); + return absl::UnknownError("ERROR"); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); @@ -61,10 +61,9 @@ TEST(AssignOrReturn, AssignsMultipleVariablesInSequence) { SAPI_ASSIGN_OR_RETURN(value3, StatusOr(3)); EXPECT_EQ(3, value3); int value4; - SAPI_ASSIGN_OR_RETURN(value4, StatusOr(absl::Status( - absl::StatusCode::kUnknown, "EXPECTED"))); - return absl::Status(absl::StatusCode::kUnknown, - absl::StrCat("ERROR: assigned value ", value4)); + SAPI_ASSIGN_OR_RETURN(value4, + StatusOr(absl::UnknownError("EXPECTED"))); + return absl::UnknownError(absl::StrCat("ERROR: assigned value ", value4)); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); @@ -77,9 +76,8 @@ TEST(AssignOrReturn, AssignsRepeatedlyToSingleVariable) { EXPECT_EQ(2, value); SAPI_ASSIGN_OR_RETURN(value, StatusOr(3)); EXPECT_EQ(3, value); - SAPI_ASSIGN_OR_RETURN(value, StatusOr(absl::Status( - absl::StatusCode::kUnknown, "EXPECTED"))); - return absl::Status(absl::StatusCode::kUnknown, "ERROR"); + SAPI_ASSIGN_OR_RETURN(value, StatusOr(absl::UnknownError("EXPECTED"))); + return absl::UnknownError("ERROR"); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); @@ -91,7 +89,7 @@ TEST(AssignOrReturn, MovesUniquePtr) { SAPI_ASSIGN_OR_RETURN( ptr, StatusOr>(absl::make_unique(1))); EXPECT_EQ(*ptr, 1); - return absl::Status(absl::StatusCode::kUnknown, "EXPECTED"); + return absl::UnknownError("EXPECTED"); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); @@ -100,8 +98,8 @@ TEST(AssignOrReturn, MovesUniquePtr) { TEST(AssignOrReturn, DoesNotAssignUniquePtrOnErrorStatus) { auto func = []() -> absl::Status { std::unique_ptr ptr; - SAPI_ASSIGN_OR_RETURN(ptr, StatusOr>(absl::Status( - absl::StatusCode::kUnknown, "EXPECTED"))); + SAPI_ASSIGN_OR_RETURN( + ptr, StatusOr>(absl::UnknownError("EXPECTED"))); EXPECT_EQ(ptr, nullptr); return absl::OkStatus(); }; @@ -118,7 +116,7 @@ TEST(AssignOrReturn, MovesUniquePtrRepeatedlyToSingleVariable) { SAPI_ASSIGN_OR_RETURN( ptr, StatusOr>(absl::make_unique(2))); EXPECT_EQ(*ptr, 2); - return absl::Status(absl::StatusCode::kUnknown, "EXPECTED"); + return absl::UnknownError("EXPECTED"); }; EXPECT_THAT(func(), StatusIs(absl::StatusCode::kUnknown, "EXPECTED")); diff --git a/sandboxed_api/util/status_matchers.h b/sandboxed_api/util/status_matchers.h index 239a5f7..c7ed2e5 100644 --- a/sandboxed_api/util/status_matchers.h +++ b/sandboxed_api/util/status_matchers.h @@ -55,7 +55,6 @@ class IsOkMatcher { class StatusIsMatcher { public: StatusIsMatcher(const StatusIsMatcher&) = default; - StatusIsMatcher& operator=(const StatusIsMatcher&) = default; StatusIsMatcher(absl::StatusCode code, absl::optional message) diff --git a/sandboxed_api/util/statusor.h b/sandboxed_api/util/statusor.h index a9597d4..f1f5be3 100644 --- a/sandboxed_api/util/statusor.h +++ b/sandboxed_api/util/statusor.h @@ -47,22 +47,27 @@ class ABSL_MUST_USE_RESULT StatusOr { StatusOr(StatusOr&&) = default; StatusOr& operator=(StatusOr&&) = default; - template - StatusOr(const StatusOr& other) : StatusOr(other) {} - - template - StatusOr(StatusOr&& other) : StatusOr(other) {} + // Not implemented: + // template StatusOr(const StatusOr& other) + // template StatusOr(StatusOr&& other) template StatusOr& operator=(const StatusOr& other) { - variant_ = other.ok() ? other.value() : other.status(); + if (other.ok()) { + variant_ = other.value(); + } else { + variant_ = other.status(); + } return *this; } template StatusOr& operator=(StatusOr&& other) { - variant_ = - other.ok() ? std::move(other).value() : std::move(other).status(); + if (other.ok()) { + variant_ = std::move(other).value(); + } else { + variant_ = std::move(other).status(); + } return *this; } @@ -71,8 +76,7 @@ class ABSL_MUST_USE_RESULT StatusOr { StatusOr(const absl::Status& status) : variant_(status) { EnsureNotOk(); } // Not implemented: - // template - // StatusOr& operator=(U&& value); + // template StatusOr& operator=(U&& value) StatusOr(T&& value) : variant_(std::move(value)) {} @@ -120,12 +124,12 @@ class ABSL_MUST_USE_RESULT StatusOr { const T&& value() const&& { EnsureOk(); - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } T&& value() && { EnsureOk(); - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } const T& ValueOrDie() const& { @@ -140,7 +144,7 @@ class ABSL_MUST_USE_RESULT StatusOr { T&& ValueOrDie() && { EnsureOk(); - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } const T& operator*() const& { @@ -155,12 +159,12 @@ class ABSL_MUST_USE_RESULT StatusOr { const T&& operator*() const&& { EnsureOk(); - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } T&& operator*() && { EnsureOk(); - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } const T* operator->() const { @@ -184,7 +188,7 @@ class ABSL_MUST_USE_RESULT StatusOr { template T value_or(U&& default_value) && { if (ok()) { - return std::move(absl::get(variant_)); + return absl::get(std::move(variant_)); } return std::forward(default_value); } diff --git a/sandboxed_api/util/statusor_test.cc b/sandboxed_api/util/statusor_test.cc index 547eaed..ba80ce1 100644 --- a/sandboxed_api/util/statusor_test.cc +++ b/sandboxed_api/util/statusor_test.cc @@ -248,8 +248,8 @@ TYPED_TEST(StatusOrTest, MoveConstructorNonOkStatus) { // status. TYPED_TEST(StatusOrTest, MoveConstructorOkStatus) { auto value = TypeParam()(); - StatusOr statusor1{value}; - StatusOr statusor2{std::move(statusor1)}; + StatusOr statusor1(value); + StatusOr statusor2(std::move(statusor1)); // The destination object should possess the value previously held by the // donor. @@ -261,7 +261,7 @@ TYPED_TEST(StatusOrTest, MoveConstructorOkStatus) { // as expected. TYPED_TEST(StatusOrTest, MoveAssignmentOperatorNonOkStatus) { absl::Status status(kErrorCode, kErrorMessage); - StatusOr statusor1{status}; + StatusOr statusor1(status); StatusOr statusor2{TypeParam()()}; // Invoke the move-assignment operator. @@ -297,7 +297,7 @@ TYPED_TEST(StatusOrTest, MoveAssignmentOperatorOkStatus) { // Verify that the sapi::IsOk() gMock matcher works with StatusOr. TYPED_TEST(StatusOrTest, IsOkMatcher) { auto value = TypeParam()(); - StatusOr statusor{value}; + StatusOr statusor(value); EXPECT_THAT(statusor, IsOk()); @@ -328,7 +328,7 @@ TEST(StatusOrTest, InitializationMoveOnlyType) { // Verify that a StatusOr object can be move-constructed from a move-only type. TEST(StatusOrTest, MoveConstructorMoveOnlyType) { auto* str = new std::string(kStringElement); - std::unique_ptr value{str}; + std::unique_ptr value(str); StatusOr> statusor1(std::move(value)); StatusOr> statusor2(std::move(statusor1));