From dde6034eac99789c4c23c5cd9f6419db02d9a649 Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Fri, 11 Feb 2022 17:26:28 -0500 Subject: [PATCH] Fix OkOrFail conversion ambiguities OkOrFail has specialized conversions for Result to avoid ambiguous implicit conversion sequences. Since user conversion operators sequences can be followed by integral promotion, specializing for integral types is necessary. Specialize ResultError so calling code() returns a status_t instead of a StatusT and message() is implemented even when not carrying a string. Eventually, these classes should be combined. Add equality operators for ResultError. Bug: 219580167 Test: atest Errors_test.cpp Merged-In: I14acecfd2aef33c40e79ddb091e2f4af9291d837 Change-Id: Ifb5ed3c2d3452b10901e4aeb19368d873225d9ce --- libutils/Errors_test.cpp | 62 +++++++++++++++ libutils/include/utils/ErrorsMacros.h | 108 +++++++++++++++++++++++++- 2 files changed, 166 insertions(+), 4 deletions(-) diff --git a/libutils/Errors_test.cpp b/libutils/Errors_test.cpp index 873c99490..0d13bb03c 100644 --- a/libutils/Errors_test.cpp +++ b/libutils/Errors_test.cpp @@ -108,3 +108,65 @@ TEST(errors, result_in_status) { status_t b = g(false); EXPECT_EQ(PERMISSION_DENIED, b); } + +TEST(errors, conversion_promotion) { + constexpr size_t successVal = 10ull; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +TEST(errors, conversion_promotion_bool) { + constexpr size_t successVal = true; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +TEST(errors, conversion_promotion_char) { + constexpr char successVal = 'a'; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value(), successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} + +struct IntContainer { + // Implicit conversion from int is desired + IntContainer(int val) : val_(val) {} + int val_; +}; + +TEST(errors, conversion_construct) { + constexpr int successVal = 10; + auto f = [&](bool success) -> Result { + OR_RETURN(success_or_fail(success)); + return successVal; + }; + auto s = f(true); + ASSERT_TRUE(s.ok()); + EXPECT_EQ(s.value().val_, successVal); + auto r = f(false); + EXPECT_TRUE(!r.ok()); + EXPECT_EQ(PERMISSION_DENIED, r.error().code()); +} diff --git a/libutils/include/utils/ErrorsMacros.h b/libutils/include/utils/ErrorsMacros.h index 048c5386f..fdc46e617 100644 --- a/libutils/include/utils/ErrorsMacros.h +++ b/libutils/include/utils/ErrorsMacros.h @@ -25,6 +25,7 @@ // [1] build/soong/cc/config/global.go#commonGlobalIncludes #include #include +#include #include @@ -44,13 +45,58 @@ struct StatusT { status_t val_; }; + namespace base { +// TODO(b/221235365) StatusT fulfill ResultError contract and cleanup. + +// Unlike typical ResultError types, the underlying code should be a status_t +// instead of a StatusT. We also special-case message generation. +template<> +struct ResultError { + ResultError(status_t s) : val_(s) { + LOG_FATAL_IF(s == OK, "Result error should not hold success"); + } + + template + operator expected>() const { + return unexpected(*this); + } + + std::string message() const { return statusToString(val_); } + status_t code() const { return val_; } + + private: + const status_t val_; +}; + +template<> +struct ResultError { + template + ResultError(T&& message, status_t s) : val_(s), message_(std::forward(message)) { + LOG_FATAL_IF(s == OK, "Result error should not hold success"); + } + + ResultError(status_t s) : val_(s) {} + + template + operator expected>() const { + return unexpected(*this); + } + + status_t code() const { return val_; } + + std::string message() const { return statusToString(val_) + message_; } + private: + const status_t val_; + std::string message_; +}; // Specialization of android::base::OkOrFail for V = status_t. This is used to use the OR_RETURN // and OR_FATAL macros with statements that yields a value of status_t. See android-base/errors.h // for the detailed contract. template <> struct OkOrFail { + static_assert(std::is_same_v); // Tests if status_t is a success value of not. static bool IsOk(const status_t& s) { return s == OK; } @@ -71,16 +117,70 @@ struct OkOrFail { // Or converts into Result. This is used when OR_RETURN is used in a function whose // return type is Result. - template >> + + template operator Result() && { - return Error(std::move(val_)); + return ResultError(std::move(val_)); } - operator Result() && { return Error(std::move(val_)); } + template + operator Result() && { + return ResultError(std::move(val_)); + } + // Since user defined conversion can be followed by numeric conversion, + // we have to specialize all conversions to results holding numeric types to + // avoid conversion ambiguities with the constructor of expected. +#pragma push_macro("SPECIALIZED_CONVERSION") +#define SPECIALIZED_CONVERSION(type)\ + operator Result() && { return ResultError(std::move(val_)); }\ + operator Result() && { return ResultError(std::move(val_));} + + SPECIALIZED_CONVERSION(int) + SPECIALIZED_CONVERSION(short int) + SPECIALIZED_CONVERSION(unsigned short int) + SPECIALIZED_CONVERSION(unsigned int) + SPECIALIZED_CONVERSION(long int) + SPECIALIZED_CONVERSION(unsigned long int) + SPECIALIZED_CONVERSION(long long int) + SPECIALIZED_CONVERSION(unsigned long long int) + SPECIALIZED_CONVERSION(bool) + SPECIALIZED_CONVERSION(char) + SPECIALIZED_CONVERSION(unsigned char) + SPECIALIZED_CONVERSION(signed char) + SPECIALIZED_CONVERSION(wchar_t) + SPECIALIZED_CONVERSION(char16_t) + SPECIALIZED_CONVERSION(char32_t) + SPECIALIZED_CONVERSION(float) + SPECIALIZED_CONVERSION(double) + SPECIALIZED_CONVERSION(long double) +#undef SPECIALIZED_CONVERSION +#pragma pop_macro("SPECIALIZED_CONVERSION") // String representation of the error value. static std::string ErrorMessage(const status_t& s) { return statusToString(s); } }; - } // namespace base + + +// These conversions make StatusT directly comparable to status_t in order to +// avoid calling code whenever comparisons are desired. + +template +bool operator==(const base::ResultError& l, const status_t& r) { + return (l.code() == r); +} +template +bool operator==(const status_t& l, const base::ResultError& r) { + return (l == r.code()); +} + +template +bool operator!=(const base::ResultError& l, const status_t& r) { + return (l.code() != r); +} +template +bool operator!=(const status_t& l, const base::ResultError& r) { + return (l != r.code()); +} + } // namespace android