Remove okay Status, rename Status->Error
Phil pointed out that we weren't actually using the okay state of the
Status type, and that it was effectively just an Error type. Rename it
to reflect that and remove the ok() methods.
Change-Id: I73644a55ec8b276b07376d4a4687fc0820837614
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/util/status.cc b/aos/util/status.cc
index de372db..dbcd4af 100644
--- a/aos/util/status.cc
+++ b/aos/util/status.cc
@@ -1,19 +1,32 @@
#include "aos/util/status.h"
namespace aos {
-Status::Status(StatusCode code, std::string_view message,
- std::optional<std::source_location> source_location)
+namespace {
+// Constructs a string view from the provided buffer if it has data and
+// otherwise uses the provided string view. Used in copy/move constructors to
+// figure out whether we should use the buffer or keep the pointer to the
+// existing std::string_view (as is the case for when we store a pointer to a
+// string literal).
+static std::string_view MakeStringViewFromBufferOrView(
+ const aos::InlinedVector<char, Error::kStaticMessageLength> &buffer,
+ const std::string_view &view) {
+ return (buffer.size() > 0) ? std::string_view(buffer.begin(), buffer.end())
+ : view;
+}
+} // namespace
+Error::Error(StatusCode code, std::string_view message,
+ std::optional<std::source_location> source_location)
: code_(code),
owned_message_(message.begin(), message.end()),
message_(owned_message_.data(), owned_message_.size()),
source_location_(std::move(source_location)) {}
-Status::Status(StatusCode code, const char *message,
- std::optional<std::source_location> source_location)
+Error::Error(StatusCode code, const char *message,
+ std::optional<std::source_location> source_location)
: code_(code),
message_(message),
source_location_(std::move(source_location)) {}
-Status::Status(Status &&other)
+Error::Error(Error &&other)
: code_(other.code_),
owned_message_(std::move(other.owned_message_)),
message_(MakeStringViewFromBufferOrView(owned_message_, other.message_)),
@@ -22,17 +35,17 @@
// buffer, we need to have a manually written move constructor to manage it.
other.message_ = {};
}
-Status &Status::operator=(Status &&other) {
+Error &Error::operator=(Error &&other) {
std::swap(*this, other);
return *this;
}
-Status::Status(const Status &other)
+Error::Error(const Error &other)
: code_(other.code_),
owned_message_(other.owned_message_),
message_(MakeStringViewFromBufferOrView(owned_message_, other.message_)),
source_location_(other.source_location_) {}
-std::string Status::ToString() const {
+std::string Error::ToString() const {
std::string source_info = "";
if (source_location_.has_value()) {
source_info = absl::StrFormat(
@@ -40,16 +53,20 @@
source_location_->line(), source_location_->function_name());
}
- return absl::StrFormat("%sStatus is %s with code of %d and message: %s",
- source_info, ok() ? "okay" : "errored", code(),
- message());
+ return absl::StrFormat("%sErrored with code of %d and message: %s",
+ source_info, code(), message());
}
template <>
-void CheckExpected<void>(const tl::expected<void, Status> &expected) {
+void CheckExpected<void>(const Result<void> &expected) {
if (expected.has_value()) {
return;
}
LOG(FATAL) << expected.error().ToString();
}
+
+int ResultExitCode(const Result<void> &expected) {
+ return expected.has_value() ? static_cast<int>(Error::StatusCode::kOk)
+ : expected.error().code();
+}
} // namespace aos
diff --git a/aos/util/status.h b/aos/util/status.h
index 314936b..fa2f4f0 100644
--- a/aos/util/status.h
+++ b/aos/util/status.h
@@ -11,32 +11,43 @@
#include "aos/containers/inlined_vector.h"
namespace aos {
-// The Status class provides a means by which errors can be readily returned
+// The Error class provides a means by which errors can be readily returned
// from methods. It will typically be wrapped by an std::expected<> to
-// accommodate a return value or the Status, although an "ok" status can also be
-// used to indicate no-error.
+// accommodate a return value or the Error.
//
-// The Status class is similar to the absl::Status or std::error_code classes,
-// in that it consists of an integer error code of some sort (where 0 indicates
-// "ok") and a string error message of some sort. The main additions of this
+// The Error class is similar to the absl::Status or std::error_code classes,
+// in that it consists of an integer error code of some sort (where 0 implicitly
+// would indicate "ok", although we assume that if there is no error then
+// you will be using an expected<> to return void or your actual return type)
+// and a string error message of some sort. The main additions of this
// class are:
// 1. Adding a first-class exposure of an std::source_location to make exposure
// of the sources of errors easier.
-// 2. Providing an interface that allows for Status implementations that expose
+// 2. Providing an interface that allows for Error implementations that expose
// messages without malloc'ing (not possible with absl::Status, although it
// is possible with std::error_code).
// 3. Making it relatively easy to quickly return a simple error & message
// (specifying a custom error with std::error_code is possible but requires
// jumping through hoops and managing some global state).
+// 4. Does not support an "okay" state, to make it clear that the user is
+// supposed to use a wrapper that will itself indicate okay.
//
-// The goal of this class is that it should be easy to convert from exiting
+// The goal of this class is that it should be easy to convert from existing
// error types (absl::Status, std::error_code) to this type.
-class Status {
+//
+// Users should typically use the Result<T> convenience method when returning
+// Errors from methods. In the case where the method would normally return void,
+// use Result<void>. Result<> is just a wrapper for tl::expected; when our
+// compilers upgrade to support std::expected this should ease the transition,
+// in addition to just providing a convenience wrapper to encourage a standard
+// pattern of use.
+class Error {
public:
// In order to allow simple error messages without memory allocation, we
// reserve a small amount of stack space for error messages. This constant
// specifies the length of these strings.
static constexpr size_t kStaticMessageLength = 128;
+
// Attaches human-readable status enums to integer codes---the specific
// numeric codes are used as exit codes when terminating execution of the
// program.
@@ -49,56 +60,54 @@
kOk = 0,
kError = 1,
};
- // Constructs a status that indicates success, with no associated error
- // message our source location.
- static Status Ok() { return Status(StatusCode::kOk, "", std::nullopt); }
+
// Constructs an Error, copying the provided message. If the message is
// shorter than kStaticMessageLength, then the message will be stored entirely
// on the stack; longer messages will require dynamic memory allocation.
// The default source_location will correspond to the call-site of the
- // Status::Error() method. This should only be overridden by wrappers that
+ // Error::Error() method. This should only be overridden by wrappers that
// want to present a fancier interface to users.
- static Status Error(
+ static Error MakeError(
std::string_view message,
std::source_location source_location = std::source_location::current()) {
- return Status(StatusCode::kError, message, std::move(source_location));
+ return Error(StatusCode::kError, message, std::move(source_location));
}
- static tl::unexpected<Status> UnexpectedError(
+ static tl::unexpected<Error> MakeUnexpectedError(
std::string_view message,
std::source_location source_location = std::source_location::current()) {
- return tl::unexpected<Status>(Error(message, std::move(source_location)));
+ return tl::unexpected<Error>(
+ MakeError(message, std::move(source_location)));
}
+
// Constructs an error, retaining the provided pointer to a null-terminated
// error message. It is assumed that the message pointer will stay valid
// ~indefinitely. This is generally only appropriate to use with string
- // literals (e.g., Status::StringLiteralError("Hello, World!")).
+ // literals (e.g., Error::StringLiteralError("Hello, World!")).
// The default source_location will correspond to the call-site of the
- // Status::Error() method. This should only be overridden by wrappers that
+ // Error::Error() method. This should only be overridden by wrappers that
// want to present a fancier interface to users.
- static Status StringLiteralError(
+ static Error MakeStringLiteralError(
const char *message,
std::source_location source_location = std::source_location::current()) {
- return Status(StatusCode::kError, message, std::move(source_location));
+ return Error(StatusCode::kError, message, std::move(source_location));
}
- static tl::unexpected<Status> UnexpectedStringLiteralError(
+ static tl::unexpected<Error> MakeUnexpectedStringLiteralError(
const char *message,
std::source_location source_location = std::source_location::current()) {
- return tl::unexpected<Status>(
- StringLiteralError(message, std::move(source_location)));
+ return tl::unexpected<Error>(
+ MakeStringLiteralError(message, std::move(source_location)));
}
- Status(Status &&other);
- Status &operator=(Status &&other);
- Status(const Status &other);
+ Error(Error &&other);
+ Error &operator=(Error &&other);
+ Error(const Error &other);
- // Returns true if the Status indicates success.
- [[nodiscard]] bool ok() const { return code_ == StatusCode::kOk; }
// Returns a numeric value for the status code. Zero will always indicate
// success; non-zero values will always indicate an error.
[[nodiscard]] int code() const { return static_cast<int>(code_); }
// Returns a view of the error message.
[[nodiscard]] std::string_view message() const { return message_; }
- // Returns the source_location attached to the current Status. If the
+ // Returns the source_location attached to the current Error. If the
// source_location was never set, will return nullopt. The source_location
// will typically be left unset for successful ("ok") statuses.
[[nodiscard]] const std::optional<std::source_location> &source_location()
@@ -109,22 +118,10 @@
std::string ToString() const;
private:
- Status(StatusCode code, std::string_view message,
- std::optional<std::source_location> source_location);
- Status(StatusCode code, const char *message,
- std::optional<std::source_location> source_location);
-
- // Constructs a string view from the provided buffer if it has data and
- // otherwise uses the provided string view. Used in copy/move constructors to
- // figure out whether we should use the buffer or keep the pointer to the
- // existing std::string_view (as is the case for when we store a pointer to a
- // string literal).
- static std::string_view MakeStringViewFromBufferOrView(
- const aos::InlinedVector<char, kStaticMessageLength> &buffer,
- const std::string_view &view) {
- return (buffer.size() > 0) ? std::string_view(buffer.begin(), buffer.end())
- : view;
- }
+ Error(StatusCode code, std::string_view message,
+ std::optional<std::source_location> source_location);
+ Error(StatusCode code, const char *message,
+ std::optional<std::source_location> source_location);
StatusCode code_;
aos::InlinedVector<char, kStaticMessageLength> owned_message_;
@@ -132,11 +129,14 @@
std::optional<std::source_location> source_location_;
};
+template <typename T>
+using Result = tl::expected<T, Error>;
+
// Dies fatally if the provided expected does not include the value T, printing
-// out an error message that includes the Status on the way out.
+// out an error message that includes the Error on the way out.
// Returns the stored value on success.
template <typename T>
-T CheckExpected(const tl::expected<T, Status> &expected) {
+T CheckExpected(const Result<T> &expected) {
if (expected.has_value()) {
return expected.value();
}
@@ -144,6 +144,8 @@
}
template <>
-void CheckExpected<void>(const tl::expected<void, Status> &expected);
+void CheckExpected<void>(const Result<void> &expected);
+
+int ResultExitCode(const Result<void> &expected);
} // namespace aos
#endif // AOS_UTIL_STATUS_H_
diff --git a/aos/util/status_test.cc b/aos/util/status_test.cc
index 46c9e40..b838636 100644
--- a/aos/util/status_test.cc
+++ b/aos/util/status_test.cc
@@ -11,38 +11,20 @@
DECLARE_bool(die_on_malloc);
namespace aos::testing {
-class StatusTest : public ::testing::Test {
+class ErrorTest : public ::testing::Test {
protected:
- StatusTest() {}
+ ErrorTest() {}
};
-// Tests that we can construct an "Ok" status and that it presents the correct
-// interface.
-TEST_F(StatusTest, Okay) {
- std::optional<Status> ok;
- {
- aos::ScopedRealtime realtime;
- ok = Status::Ok();
- }
- ASSERT_TRUE(ok.has_value());
- EXPECT_TRUE(ok->ok());
- EXPECT_EQ(0, ok->code());
- EXPECT_EQ("", ok->message());
- EXPECT_FALSE(ok->source_location().has_value());
- EXPECT_EQ(std::string("Status is okay with code of 0 and message: "),
- ok->ToString());
-}
-
// Tests that we can construct an errored status in realtime code.
-TEST_F(StatusTest, RealtimeError) {
- std::optional<Status> error;
+TEST_F(ErrorTest, RealtimeError) {
+ std::optional<Error> error;
{
aos::ScopedRealtime realtime;
- error = Status::Error("Hello, World!");
+ error = Error::MakeError("Hello, World!");
}
const int line = __LINE__ - 2;
ASSERT_TRUE(error.has_value());
- EXPECT_FALSE(error->ok());
EXPECT_NE(0, error->code());
EXPECT_EQ(std::string("Hello, World!"), error->message());
ASSERT_TRUE(error->source_location().has_value());
@@ -51,7 +33,7 @@
std::filesystem::path(error->source_location()->file_name()).filename());
EXPECT_EQ(
std::string("virtual void "
- "aos::testing::StatusTest_RealtimeError_Test::TestBody()"),
+ "aos::testing::ErrorTest_RealtimeError_Test::TestBody()"),
error->source_location()->function_name());
EXPECT_EQ(line, error->source_location()->line());
EXPECT_LT(1, error->source_location()->column());
@@ -59,22 +41,32 @@
error->ToString(),
::testing::HasSubstr(absl::StrFormat(
"status_test.cc:%d in virtual void "
- "aos::testing::StatusTest_RealtimeError_Test::TestBody(): Status is "
- "errored with code of 1 and message: Hello, World!",
+ "aos::testing::ErrorTest_RealtimeError_Test::TestBody(): Errored "
+ "with code of 1 and message: Hello, World!",
line)));
}
+// Tests that the ResultExitCode() function will correctly transform a Result<>
+// object into an exit code suitable for exiting a program.
+TEST_F(ErrorTest, ExitCode) {
+ static_assert(0 == static_cast<int>(Error::StatusCode::kOk));
+ EXPECT_EQ(static_cast<int>(Error::StatusCode::kOk),
+ ResultExitCode(Result<void>{}));
+ EXPECT_EQ(static_cast<int>(Error::StatusCode::kError),
+ ResultExitCode(Error::MakeUnexpectedError("")));
+}
+
// Malloc hooks don't work with asan/msan.
#if !__has_feature(address_sanitizer) && !__has_feature(memory_sanitizer)
// Tests that we do indeed malloc (and catch it) on an extra-long error message
// (this is mostly intended to ensure that the test setup is working correctly).
-TEST(StatusDeathTest, BlowsUpOnRealtimeAllocation) {
- std::string message(" ", Status::kStaticMessageLength + 1);
+TEST(ErrorDeathTest, BlowsUpOnRealtimeAllocation) {
+ std::string message(" ", Error::kStaticMessageLength + 1);
EXPECT_DEATH(
{
aos::ScopedRealtime realtime;
aos::CheckRealtime();
- Status foo = Status::Error(message);
+ Error foo = Error::MakeError(message);
},
"Malloced");
}
@@ -82,20 +74,19 @@
#endif
// Tests that we can use arbitrarily-sized string literals for error messages.
-TEST_F(StatusTest, StringLiteralError) {
- std::optional<Status> error;
+TEST_F(ErrorTest, StringLiteralError) {
+ std::optional<Error> error;
const char *message =
"Hellllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllll"
"llllllllllllllloooooooooooooooooooooooooooooooooooooooooooo, "
"World!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
"!!!!!!!!!!!!!!";
- ASSERT_LT(Status::kStaticMessageLength, strlen(message));
+ ASSERT_LT(Error::kStaticMessageLength, strlen(message));
{
aos::ScopedRealtime realtime;
- error = Status::StringLiteralError(message);
+ error = Error::MakeStringLiteralError(message);
}
ASSERT_TRUE(error.has_value());
- EXPECT_FALSE(error->ok());
EXPECT_EQ(message, error->message());
ASSERT_TRUE(error->source_location().has_value());
EXPECT_EQ(
@@ -104,16 +95,16 @@
}
// Tests that the CheckExpected() call works as intended.
-TEST(StatusDeathTest, CheckExpected) {
- tl::expected<int, Status> expected;
+TEST(ErrorDeathTest, CheckExpected) {
+ tl::expected<int, Error> expected;
expected.emplace(971);
EXPECT_EQ(971, CheckExpected(expected))
<< "Should have gotten out the emplaced value on no error.";
- expected = Status::UnexpectedError("Hello, World!");
+ expected = Error::MakeUnexpectedError("Hello, World!");
EXPECT_DEATH(CheckExpected(expected), "Hello, World!")
<< "An error message including the error string should have been printed "
"on death.";
- EXPECT_DEATH(CheckExpected<void>(Status::UnexpectedError("void expected")),
+ EXPECT_DEATH(CheckExpected<void>(Error::MakeUnexpectedError("void expected")),
"void expected")
<< "A void expected should work with CheckExpected().";
}