diff --git a/mediapipe/framework/deps/status_builder.cc b/mediapipe/framework/deps/status_builder.cc index 8358ea01a..70775949d 100644 --- a/mediapipe/framework/deps/status_builder.cc +++ b/mediapipe/framework/deps/status_builder.cc @@ -14,45 +14,43 @@ #include "mediapipe/framework/deps/status_builder.h" +#include +#include + #include "absl/memory/memory.h" #include "absl/status/status.h" +#include "absl/strings/str_cat.h" namespace mediapipe { -StatusBuilder::StatusBuilder(const StatusBuilder& sb) { - status_ = sb.status_; - file_ = sb.file_; - line_ = sb.line_; - no_logging_ = sb.no_logging_; - stream_ = sb.stream_ - ? absl::make_unique(sb.stream_->str()) - : nullptr; - join_style_ = sb.join_style_; -} +StatusBuilder::StatusBuilder(const StatusBuilder& sb) + : impl_(sb.impl_ ? std::make_unique(*sb.impl_) : nullptr) {} StatusBuilder& StatusBuilder::operator=(const StatusBuilder& sb) { - status_ = sb.status_; - file_ = sb.file_; - line_ = sb.line_; - no_logging_ = sb.no_logging_; - stream_ = sb.stream_ - ? absl::make_unique(sb.stream_->str()) - : nullptr; - join_style_ = sb.join_style_; + if (!sb.impl_) { + impl_ = nullptr; + return *this; + } + if (impl_) { + *impl_ = *sb.impl_; + return *this; + } + impl_ = std::make_unique(*sb.impl_); + return *this; } StatusBuilder& StatusBuilder::SetAppend() & { - if (status_.ok()) return *this; - join_style_ = MessageJoinStyle::kAppend; + if (!impl_) return *this; + impl_->join_style = Impl::MessageJoinStyle::kAppend; return *this; } StatusBuilder&& StatusBuilder::SetAppend() && { return std::move(SetAppend()); } StatusBuilder& StatusBuilder::SetPrepend() & { - if (status_.ok()) return *this; - join_style_ = MessageJoinStyle::kPrepend; + if (!impl_) return *this; + impl_->join_style = Impl::MessageJoinStyle::kPrepend; return *this; } @@ -61,7 +59,8 @@ StatusBuilder&& StatusBuilder::SetPrepend() && { } StatusBuilder& StatusBuilder::SetNoLogging() & { - no_logging_ = true; + if (!impl_) return *this; + impl_->no_logging = true; return *this; } @@ -70,34 +69,72 @@ StatusBuilder&& StatusBuilder::SetNoLogging() && { } StatusBuilder::operator Status() const& { - if (!stream_ || stream_->str().empty() || no_logging_) { - return status_; - } return StatusBuilder(*this).JoinMessageToStatus(); } -StatusBuilder::operator Status() && { - if (!stream_ || stream_->str().empty() || no_logging_) { - return status_; - } - return JoinMessageToStatus(); -} +StatusBuilder::operator Status() && { return JoinMessageToStatus(); } absl::Status StatusBuilder::JoinMessageToStatus() { - if (!stream_) { + if (!impl_) { return absl::OkStatus(); } - std::string message; - if (join_style_ == MessageJoinStyle::kAnnotate) { - if (!status_.ok()) { - message = absl::StrCat(status_.message(), "; ", stream_->str()); - } - } else { - message = join_style_ == MessageJoinStyle::kPrepend - ? absl::StrCat(stream_->str(), status_.message()) - : absl::StrCat(status_.message(), stream_->str()); + return impl_->JoinMessageToStatus(); +} + +absl::Status StatusBuilder::Impl::JoinMessageToStatus() { + if (stream.str().empty() || no_logging) { + return status; } - return Status(status_.code(), message); + return absl::Status(status.code(), [this]() { + switch (join_style) { + case MessageJoinStyle::kAnnotate: + return absl::StrCat(status.message(), "; ", stream.str()); + case MessageJoinStyle::kAppend: + return absl::StrCat(status.message(), stream.str()); + case MessageJoinStyle::kPrepend: + return absl::StrCat(stream.str(), status.message()); + } + }()); +} + +StatusBuilder::Impl::Impl(const absl::Status& status, const char* file, + int line) + : status(status), line(line), file(file), stream() {} + +StatusBuilder::Impl::Impl(absl::Status&& status, const char* file, int line) + : status(std::move(status)), line(line), file(file), stream() {} + +StatusBuilder::Impl::Impl(const absl::Status& status, + mediapipe::source_location location) + : status(status), + line(location.line()), + file(location.file_name()), + stream() {} + +StatusBuilder::Impl::Impl(absl::Status&& status, + mediapipe::source_location location) + : status(std::move(status)), + line(location.line()), + file(location.file_name()), + stream() {} + +StatusBuilder::Impl::Impl(const Impl& other) + : status(other.status), + line(other.line), + file(other.file), + no_logging(other.no_logging), + stream(other.stream.str()), + join_style(other.join_style) {} + +StatusBuilder::Impl& StatusBuilder::Impl::operator=(const Impl& other) { + status = other.status; + line = other.line; + file = other.file; + no_logging = other.no_logging; + stream = std::ostringstream(other.stream.str()); + join_style = other.join_style; + + return *this; } } // namespace mediapipe diff --git a/mediapipe/framework/deps/status_builder.h b/mediapipe/framework/deps/status_builder.h index c9111c603..d2e40d575 100644 --- a/mediapipe/framework/deps/status_builder.h +++ b/mediapipe/framework/deps/status_builder.h @@ -22,7 +22,6 @@ #include "absl/base/attributes.h" #include "absl/memory/memory.h" #include "absl/status/status.h" -#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "mediapipe/framework/deps/source_location.h" #include "mediapipe/framework/deps/status.h" @@ -42,34 +41,37 @@ class ABSL_MUST_USE_RESULT StatusBuilder { // occurs. A typical user will call this with `MEDIAPIPE_LOC`. StatusBuilder(const absl::Status& original_status, mediapipe::source_location location) - : status_(original_status), - line_(location.line()), - file_(location.file_name()), - stream_(InitStream(status_)) {} + : impl_(original_status.ok() + ? nullptr + : std::make_unique(original_status, location)) {} StatusBuilder(absl::Status&& original_status, mediapipe::source_location location) - : status_(std::move(original_status)), - line_(location.line()), - file_(location.file_name()), - stream_(InitStream(status_)) {} + : impl_(original_status.ok() + ? nullptr + : std::make_unique(std::move(original_status), + location)) {} // Creates a `StatusBuilder` from a mediapipe status code. If logging is // enabled, it will use `location` as the location from which the log message // occurs. A typical user will call this with `MEDIAPIPE_LOC`. StatusBuilder(absl::StatusCode code, mediapipe::source_location location) - : status_(code, ""), - line_(location.line()), - file_(location.file_name()), - stream_(InitStream(status_)) {} + : impl_(code == absl::StatusCode::kOk + ? nullptr + : std::make_unique(absl::Status(code, ""), location)) {} StatusBuilder(const absl::Status& original_status, const char* file, int line) - : status_(original_status), - line_(line), - file_(file), - stream_(InitStream(status_)) {} + : impl_(original_status.ok() + ? nullptr + : std::make_unique(original_status, file, line)) {} - bool ok() const { return status_.ok(); } + StatusBuilder(absl::Status&& original_status, const char* file, int line) + : impl_(original_status.ok() + ? nullptr + : std::make_unique(std::move(original_status), file, + line)) {} + + bool ok() const { return !impl_; } StatusBuilder& SetAppend() &; StatusBuilder&& SetAppend() &&; @@ -82,8 +84,8 @@ class ABSL_MUST_USE_RESULT StatusBuilder { template StatusBuilder& operator<<(const T& msg) & { - if (!stream_) return *this; - *stream_ << msg; + if (!impl_) return *this; + impl_->stream << msg; return *this; } @@ -98,35 +100,42 @@ class ABSL_MUST_USE_RESULT StatusBuilder { absl::Status JoinMessageToStatus(); private: - // Specifies how to join the error message in the original status and any - // additional message that has been streamed into the builder. - enum class MessageJoinStyle { - kAnnotate, - kAppend, - kPrepend, + struct Impl { + // Specifies how to join the error message in the original status and any + // additional message that has been streamed into the builder. + enum class MessageJoinStyle { + kAnnotate, + kAppend, + kPrepend, + }; + + Impl(const absl::Status& status, const char* file, int line); + Impl(absl::Status&& status, const char* file, int line); + Impl(const absl::Status& status, mediapipe::source_location location); + Impl(absl::Status&& status, mediapipe::source_location location); + Impl(const Impl&); + Impl& operator=(const Impl&); + + absl::Status JoinMessageToStatus(); + + // The status that the result will be based on. + absl::Status status; + // The line to record if this file is logged. + int line; + // Not-owned: The file to record if this status is logged. + const char* file; + // Logging disabled if true. + bool no_logging = false; + // The additional messages added with `<<`. This is nullptr when status_ is + // ok. + std::ostringstream stream; + // Specifies how to join the message in `status_` and `stream_`. + MessageJoinStyle join_style = MessageJoinStyle::kAnnotate; }; - // Conditionally creates an ostringstream if the status is not ok. - static std::unique_ptr InitStream( - const absl::Status status) { - if (status.ok()) { - return nullptr; - } - return absl::make_unique(); - } - - // The status that the result will be based on. - absl::Status status_; - // The line to record if this file is logged. - int line_; - // Not-owned: The file to record if this status is logged. - const char* file_; - bool no_logging_ = false; - // The additional messages added with `<<`. This is nullptr when status_ is - // ok. - std::unique_ptr stream_; - // Specifies how to join the message in `status_` and `stream_`. - MessageJoinStyle join_style_ = MessageJoinStyle::kAnnotate; + // Internal store of data for the class. An invariant of the class is that + // this is null when the original status is okay, and not-null otherwise. + std::unique_ptr impl_; }; inline StatusBuilder AlreadyExistsErrorBuilder( diff --git a/mediapipe/framework/deps/status_builder_test.cc b/mediapipe/framework/deps/status_builder_test.cc index f517bb909..560acd3c6 100644 --- a/mediapipe/framework/deps/status_builder_test.cc +++ b/mediapipe/framework/deps/status_builder_test.cc @@ -33,6 +33,21 @@ TEST(StatusBuilder, OkStatusRvalue) { ASSERT_EQ(status, absl::OkStatus()); } +TEST(StatusBuilder, OkStatusFileAndLineRvalueStatus) { + absl::Status status = StatusBuilder(absl::OkStatus(), "hello.cc", 1234) + << "annotated message1 " + << "annotated message2"; + ASSERT_EQ(status, absl::OkStatus()); +} + +TEST(StatusBuilder, OkStatusFileAndLineLvalueStatus) { + const auto original_status = absl::OkStatus(); + absl::Status status = StatusBuilder(original_status, "hello.cc", 1234) + << "annotated message1 " + << "annotated message2"; + ASSERT_EQ(status, absl::OkStatus()); +} + TEST(StatusBuilder, AnnotateMode) { absl::Status status = StatusBuilder(absl::Status(absl::StatusCode::kNotFound, "original message"), @@ -45,6 +60,30 @@ TEST(StatusBuilder, AnnotateMode) { "original message; annotated message1 annotated message2"); } +TEST(StatusBuilder, AnnotateModeFileAndLineRvalueStatus) { + absl::Status status = StatusBuilder(absl::Status(absl::StatusCode::kNotFound, + "original message"), + "hello.cc", 1234) + << "annotated message1 " + << "annotated message2"; + ASSERT_FALSE(status.ok()); + EXPECT_EQ(status.code(), absl::StatusCode::kNotFound); + EXPECT_EQ(status.message(), + "original message; annotated message1 annotated message2"); +} + +TEST(StatusBuilder, AnnotateModeFileAndLineLvalueStatus) { + const auto original_status = + absl::Status(absl::StatusCode::kNotFound, "original message"); + absl::Status status = StatusBuilder(original_status, "hello.cc", 1234) + << "annotated message1 " + << "annotated message2"; + ASSERT_FALSE(status.ok()); + EXPECT_EQ(status.code(), absl::StatusCode::kNotFound); + EXPECT_EQ(status.message(), + "original message; annotated message1 annotated message2"); +} + TEST(StatusBuilder, PrependModeLvalue) { StatusBuilder builder( absl::Status(absl::StatusCode::kInvalidArgument, "original message"),