Internal change

PiperOrigin-RevId: 486547910

mediapipe/framework/deps/status_builder.cc

@@ -14,45 +14,43 @@
 
 #include "mediapipe/framework/deps/status_builder.h"
 
+#include <memory>
+#include <sstream>
+
 #include "absl/memory/memory.h"
 #include "absl/status/status.h"
+#include "absl/strings/str_cat.h"
 
 namespace mediapipe {
 
-StatusBuilder::StatusBuilder(const StatusBuilder& sb) {
+StatusBuilder::StatusBuilder(const StatusBuilder& sb)
-  status_ = sb.status_;
+    : impl_(sb.impl_ ? std::make_unique<Impl>(*sb.impl_) : nullptr) {}
-  file_ = sb.file_;
-  line_ = sb.line_;
-  no_logging_ = sb.no_logging_;
-  stream_ = sb.stream_
-                ? absl::make_unique<std::ostringstream>(sb.stream_->str())
-                : nullptr;
-  join_style_ = sb.join_style_;
-}
 
 StatusBuilder& StatusBuilder::operator=(const StatusBuilder& sb) {
-  status_ = sb.status_;
+  if (!sb.impl_) {
-  file_ = sb.file_;
+    impl_ = nullptr;
-  line_ = sb.line_;
+    return *this;
-  no_logging_ = sb.no_logging_;
+  }
-  stream_ = sb.stream_
+  if (impl_) {
-                ? absl::make_unique<std::ostringstream>(sb.stream_->str())
+    *impl_ = *sb.impl_;
-                : nullptr;
+    return *this;
-  join_style_ = sb.join_style_;
+  }
+  impl_ = std::make_unique<Impl>(*sb.impl_);
+
   return *this;
 }
 
 StatusBuilder& StatusBuilder::SetAppend() & {
-  if (status_.ok()) return *this;
+  if (!impl_) return *this;
-  join_style_ = MessageJoinStyle::kAppend;
+  impl_->join_style = Impl::MessageJoinStyle::kAppend;
   return *this;
 }
 
 StatusBuilder&& StatusBuilder::SetAppend() && { return std::move(SetAppend()); }
 
 StatusBuilder& StatusBuilder::SetPrepend() & {
-  if (status_.ok()) return *this;
+  if (!impl_) return *this;
-  join_style_ = MessageJoinStyle::kPrepend;
+  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() && {
+StatusBuilder::operator Status() && { return JoinMessageToStatus(); }
-  if (!stream_ || stream_->str().empty() || no_logging_) {
-    return status_;
-  }
-  return JoinMessageToStatus();
-}
 
 absl::Status StatusBuilder::JoinMessageToStatus() {
-  if (!stream_) {
+  if (!impl_) {
     return absl::OkStatus();
   }
-  std::string message;
+  return impl_->JoinMessageToStatus();
-  if (join_style_ == MessageJoinStyle::kAnnotate) {
-    if (!status_.ok()) {
-      message = absl::StrCat(status_.message(), "; ", stream_->str());
 }
-  } else {
+
-    message = join_style_ == MessageJoinStyle::kPrepend
+absl::Status StatusBuilder::Impl::JoinMessageToStatus() {
-                  ? absl::StrCat(stream_->str(), status_.message())
+  if (stream.str().empty() || no_logging) {
-                  : absl::StrCat(status_.message(), stream_->str());
+    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

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),
+      : impl_(original_status.ok()
-        line_(location.line()),
+                  ? nullptr
-        file_(location.file_name()),
+                  : std::make_unique<Impl>(original_status, location)) {}
-        stream_(InitStream(status_)) {}
 
   StatusBuilder(absl::Status&& original_status,
                 mediapipe::source_location location)
-      : status_(std::move(original_status)),
+      : impl_(original_status.ok()
-        line_(location.line()),
+                  ? nullptr
-        file_(location.file_name()),
+                  : std::make_unique<Impl>(std::move(original_status),
-        stream_(InitStream(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, ""),
+      : impl_(code == absl::StatusCode::kOk
-        line_(location.line()),
+                  ? nullptr
-        file_(location.file_name()),
+                  : std::make_unique<Impl>(absl::Status(code, ""), location)) {}
-        stream_(InitStream(status_)) {}
 
   StatusBuilder(const absl::Status& original_status, const char* file, int line)
-      : status_(original_status),
+      : impl_(original_status.ok()
-        line_(line),
+                  ? nullptr
-        file_(file),
+                  : std::make_unique<Impl>(original_status, file, line)) {}
-        stream_(InitStream(status_)) {}
 
-  bool ok() const { return status_.ok(); }
+  StatusBuilder(absl::Status&& original_status, const char* file, int line)
+      : impl_(original_status.ok()
+                  ? nullptr
+                  : std::make_unique<Impl>(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 <typename T>
   StatusBuilder& operator<<(const T& msg) & {
-    if (!stream_) return *this;
+    if (!impl_) return *this;
-    *stream_ << msg;
+    impl_->stream << msg;
     return *this;
   }
 
@@ -98,6 +100,7 @@ class ABSL_MUST_USE_RESULT StatusBuilder {
   absl::Status JoinMessageToStatus();
 
  private:
+  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 {
@@ -106,27 +109,33 @@ class ABSL_MUST_USE_RESULT StatusBuilder {
       kPrepend,
     };
 
-  // Conditionally creates an ostringstream if the status is not ok.
+    Impl(const absl::Status& status, const char* file, int line);
-  static std::unique_ptr<std::ostringstream> InitStream(
+    Impl(absl::Status&& status, const char* file, int line);
-      const absl::Status status) {
+    Impl(const absl::Status& status, mediapipe::source_location location);
-    if (status.ok()) {
+    Impl(absl::Status&& status, mediapipe::source_location location);
-      return nullptr;
+    Impl(const Impl&);
-    }
+    Impl& operator=(const Impl&);
-    return absl::make_unique<std::ostringstream>();
+
-  }
+    absl::Status JoinMessageToStatus();
 
     // The status that the result will be based on.
-  absl::Status status_;
+    absl::Status status;
     // The line to record if this file is logged.
-  int line_;
+    int line;
     // Not-owned: The file to record if this status is logged.
-  const char* file_;
+    const char* file;
-  bool no_logging_ = false;
+    // Logging disabled if true.
+    bool no_logging = false;
     // The additional messages added with `<<`.  This is nullptr when status_ is
     // ok.
-  std::unique_ptr<std::ostringstream> stream_;
+    std::ostringstream stream;
     // Specifies how to join the message in `status_` and `stream_`.
-  MessageJoinStyle join_style_ = MessageJoinStyle::kAnnotate;
+    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> impl_;
 };
 
 inline StatusBuilder AlreadyExistsErrorBuilder(

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"),