Add move operations to Status. (#3773)

Author: wu-hui

Firestore/core/src/firebase/firestore/util/status.cc

@@ -28,9 +28,7 @@ namespace util {
 
 Status::Status(Error code, absl::string_view msg) {
   HARD_ASSERT(code != Error::Ok);
-  state_ = absl::make_unique<State>();
-  state_->code = code;
-  state_->msg = static_cast<std::string>(msg);
+  state_ = State::MakePtr(code, static_cast<std::string>(msg));
 }
 
 void Status::Update(const Status& new_status) {
@@ -40,15 +38,16 @@ void Status::Update(const Status& new_status) {
 }
 
 Status& Status::CausedBy(const Status& cause) {
-  if (cause.ok() || this == &cause) {
+  if (cause.ok() || this == &cause || cause.IsMovedFrom()) {
     return *this;
   }
 
-  if (ok()) {
+  if (ok() || IsMovedFrom()) {
     *this = cause;
     return *this;
   }
 
+  std::string new_message = error_message();
   absl::StrAppend(&state_->msg, ": ", cause.error_message());
 
   // If this Status has no accompanying PlatformError but the cause does, create
@@ -65,15 +64,30 @@ Status& Status::CausedBy(const Status& cause) {
 
 Status& Status::WithPlatformError(std::unique_ptr<PlatformError> error) {
   HARD_ASSERT(!ok(), "Platform errors should not be applied to Status::OK()");
+  if (IsMovedFrom()) {
+    std::string message = moved_from_message();
+    state_ = State::MakePtr(Error::Internal, std::move(message));
+  }
   state_->platform_error = std::move(error);
   return *this;
 }
 
+void Status::State::Deleter::operator()(const State* ptr) const {
+  if (ptr != State::MovedFromIndicator()) {
+    delete ptr;
+  }
+}
+
+void Status::SetMovedFrom() {
+  // Set pointer value to `0x1` as the pointer is no longer useful.
+  state_ = State::StatePtr{State::MovedFromIndicator()};
+}
+
 void Status::SlowCopyFrom(const State* src) {
   if (src == nullptr) {
     state_ = nullptr;
   } else {
-    state_ = absl::make_unique<State>(*src);
+    state_ = State::MakePtr(*src);
   }
 }
 
@@ -82,6 +96,11 @@ const std::string& Status::empty_string() {
   return *empty;
 }
 
+const std::string& Status::moved_from_message() {
+  static std::string* message = new std::string("Status accessed after move.");
+  return *message;
+}
+
 std::string Status::ToString() const {
   if (state_ == nullptr) {
     return "OK";
@@ -141,7 +160,7 @@ std::string Status::ToString() const {
         break;
     }
     result += ": ";
-    result += state_->msg;
+    result += IsMovedFrom() ? moved_from_message() : state_->msg;
     return result;
   }
 }

Firestore/core/src/firebase/firestore/util/status.h

@@ -25,6 +25,7 @@
 #include <iosfwd>
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "Firestore/core/include/firebase/firestore/firestore_errors.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
@@ -53,6 +54,10 @@ class ABSL_MUST_USE_RESULT Status {
   Status(const Status& s);
   void operator=(const Status& s);
 
+  /// Move the specified status.
+  Status(Status&& s) noexcept;
+  void operator=(Status&& s) noexcept;
+
   static Status OK() {
     return Status();
   }
@@ -72,15 +77,16 @@ class ABSL_MUST_USE_RESULT Status {
 
   /// Returns true iff the status indicates success.
   bool ok() const {
-    return (state_ == nullptr);
+    return state_ == nullptr;
   }
 
   Error code() const {
-    return ok() ? Error::Ok : state_->code;
+    return ok() ? Error::Ok : (IsMovedFrom() ? Error::Internal : state_->code);
   }
 
   const std::string& error_message() const {
-    return ok() ? empty_string() : state_->msg;
+    return ok() ? empty_string()
+                : (IsMovedFrom() ? moved_from_message() : state_->msg);
   }
 
   bool operator==(const Status& x) const;
@@ -116,9 +122,29 @@ class ABSL_MUST_USE_RESULT Status {
 
  private:
   static const std::string& empty_string();
+  static const std::string& moved_from_message();
+
   struct State {
     State() = default;
     State(const State& other);
+    State(Error code, std::string&& msg);
+
+    struct Deleter {
+      void operator()(const State* ptr) const;
+    };
+    // A `unique_ptr` with a custom deleter. If the pointer's value has been set
+    // to a special value (0x01) to indicate it is moved, invoking the custom
+    // deleter will be a no-op.
+    using StatePtr = std::unique_ptr<State, Deleter>;
+
+    static State* MovedFromIndicator() {
+      return reinterpret_cast<State*>(0x01);
+    }
+
+    template <typename... Args>
+    static StatePtr MakePtr(Args&&... args) {
+      return StatePtr(new State(std::forward<Args>(args)...));
+    }
 
     Error code;
     std::string msg;
@@ -129,9 +155,20 @@ class ABSL_MUST_USE_RESULT Status {
     // NSError* to Status and back to NSError* losslessly.
     std::unique_ptr<PlatformError> platform_error;
   };
-  // OK status has a `nullptr` state_.  Otherwise, `state_` points to
-  // a `State` structure containing the error code and message(s)
-  std::unique_ptr<State> state_;
+
+  // Asserts if `state_` is a valid pointer, should be used at all places where
+  // it is used as a pointer, instead of using `state_`.
+  bool IsMovedFrom() const {
+    return state_.get() == State::MovedFromIndicator();
+  }
+
+  // OK status has a `nullptr` `state_`. If this instance is moved, state_ has
+  // the value of `State::MovedFromIndicator()`. Otherwise `state_` points to
+  // a `State` structure containing the error code and message(s).
+  State::StatePtr state_;
+
+  // Tags this instance as `moved-from`.
+  void SetMovedFrom();
 
   void SlowCopyFrom(const State* src);
 };
@@ -152,7 +189,8 @@ class PlatformError {
 };
 
 inline Status::Status(const Status& s)
-    : state_((s.state_ == nullptr) ? nullptr : new State(*s.state_)) {
+    : state_{s.state_ == nullptr ? State::StatePtr{}
+                                 : State::MakePtr(*s.state_)} {
 }
 
 inline Status::State::State(const State& s)
@@ -162,6 +200,10 @@ inline Status::State::State(const State& s)
                                                    : s.platform_error->Copy()) {
 }
 
+inline Status::State::State(Error code, std::string&& msg)
+    : code(code), msg(std::move(msg)) {
+}
+
 inline void Status::operator=(const Status& s) {
   // The following condition catches both aliasing (when this == &s),
   // and the common case where both s and *this are ok.
@@ -170,6 +212,18 @@ inline void Status::operator=(const Status& s) {
   }
 }
 
+inline Status::Status(Status&& s) noexcept : state_(std::move(s.state_)) {
+  s.SetMovedFrom();
+}
+
+inline void Status::operator=(Status&& s) noexcept {
+  // Moving into self is a no-op.
+  if (this != &s) {
+    state_ = std::move(s.state_);
+    s.SetMovedFrom();
+  }
+}
+
 inline bool Status::operator==(const Status& x) const {
   return (this->state_ == x.state_) || (ToString() == x.ToString());
 }

Firestore/core/src/firebase/firestore/util/status_apple.mm

@@ -110,6 +110,8 @@ Status FromFirestoreNSError(NSError* error) {
 
 NSError* Status::ToNSError() const {
   if (ok()) return nil;
+  // Early exit because `state_` is moved.
+  if (IsMovedFrom()) return MakeNSError(code(), error_message());
 
   NSError* error = UnderlyingNSError::Recover(state_->platform_error);
   if (error) return error;

Firestore/core/test/firebase/firestore/util/status_apple_test.mm

@@ -86,6 +86,14 @@
   EXPECT_TRUE([not_found_nserror isEqual:cause]);
 }
 
+TEST(Status, MovedFromToNSError) {
+  Status not_found(Error::NotFound, "Some file not found");
+  Status unused = std::move(not_found);
+
+  EXPECT_EQ(not_found.ToNSError().domain, FIRFirestoreErrorDomain);
+  EXPECT_EQ(not_found.ToNSError().code, Internal);
+}
+
 }  // namespace util
 }  // namespace firestore
 }  // namespace firebase

Firestore/core/test/firebase/firestore/util/status_test.cc

@@ -55,13 +55,55 @@ TEST(Status, Copy) {
   ASSERT_EQ(a.ToString(), b.ToString());
 }
 
+TEST(Status, Move) {
+  Status s(Status(Error::InvalidArgument, "Invalid"));
+  ASSERT_EQ(Error::InvalidArgument, s.code());
+
+  Status new_s = std::move(s);
+  ASSERT_EQ(Error::InvalidArgument, new_s.code());
+
+  Status ok = Status::OK();
+  Status new_ok = std::move(ok);
+  ASSERT_TRUE(new_ok.ok());
+  // Moved OK is not OK anymore.
+  ASSERT_FALSE(ok.ok());
+}
+
 TEST(Status, Assign) {
   Status a(Error::InvalidArgument, "Invalid");
   Status b;
   b = a;
   ASSERT_EQ(a.ToString(), b.ToString());
 }
 
+TEST(Status, MoveAssign) {
+  Status ok;
+  Status reassigned{Error::InvalidArgument, "Foo"};
+  reassigned = std::move(ok);
+  ASSERT_EQ(reassigned, Status::OK());
+
+  Status bad{Error::InvalidArgument, "Foo"};
+  reassigned = std::move(bad);
+  ASSERT_EQ(reassigned, Status(Error::InvalidArgument, "Foo"));
+}
+
+TEST(Status, CanAccessMovedFrom) {
+  Status ok = Status::OK();
+  Status assigned = std::move(ok);
+
+  ASSERT_FALSE(ok.ok());
+  ASSERT_EQ(ok.error_message(), "Status accessed after move.");
+  ASSERT_EQ(ok.code(), Error::Internal);
+}
+
+TEST(Status, CanAssignToMovedFromStatus) {
+  Status a(Error::InvalidArgument, "Invalid");
+  Status b = std::move(a);
+
+  EXPECT_NO_THROW({ a = Status(Error::Internal, "Internal"); });
+  ASSERT_EQ(a.ToString(), "Internal: Internal");
+}
+
 TEST(Status, Update) {
   Status s;
   s.Update(Status::OK());
@@ -77,6 +119,14 @@ TEST(Status, Update) {
   ASSERT_FALSE(s.ok());
 }
 
+TEST(Status, CanUpdateMovedFrom) {
+  Status a(Error::InvalidArgument, "Invalid");
+  Status b = std::move(a);
+
+  a.Update(b);
+  ASSERT_EQ(a.ToString(), "Internal: Status accessed after move.");
+}
+
 TEST(Status, EqualsOK) {
   ASSERT_EQ(Status::OK(), Status());
 }
@@ -105,6 +155,17 @@ TEST(Status, EqualsDifferentMessage) {
   ASSERT_NE(a, b);
 }
 
+TEST(Status, EqualsApplyToMovedFrom) {
+  Status a(Error::InvalidArgument, "message");
+  Status unused = std::move(a);
+  Status b(Error::InvalidArgument, "message");
+
+  ASSERT_NE(a, b);
+
+  unused = std::move(b);
+  ASSERT_EQ(a, b);
+}
+
 TEST(Status, FromErrno) {
   Status a = Status::FromErrno(EEXIST, "Cannot write file");
   ASSERT_THAT(
@@ -159,6 +220,15 @@ TEST(Status, CauseBy_Self) {
   EXPECT_EQ(not_found, result);
 }
 
+TEST(Status, CauseBy_OnMovedFrom) {
+  Status not_ready(Error::FailedPrecondition, "DB not ready");
+  Status not_found(Error::NotFound, "file not found");
+  Status unused = std::move(not_ready);
+
+  Status result = not_ready.CausedBy(not_found);
+  EXPECT_EQ(not_found, result);
+}
+
 }  // namespace util
 }  // namespace firestore
 }  // namespace firebase