review comments

apache · Nov 4, 2024 · 3000e96 · 3000e96
1 parent f7d3fff
commit 3000e96
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 15 deletions.
diff --git a/cpp/src/arrow/status.cc b/cpp/src/arrow/status.cc
@@ -31,12 +31,12 @@ Status::Status(StatusCode code, std::string msg, std::shared_ptr<StatusDetail> d
   state_ = new State{code, std::move(msg), std::move(detail)};
 }
 
-// We would prefer that this destructor *not* be inlined, since the vast majority of
-// Statuses are OK and so inlining would superflously increase binary size.
-Status::State::~State() = default;
-
 void Status::CopyFrom(const Status& s) {
-  DeleteState();
+  if (ARROW_PREDICT_FALSE(state_ != NULL)) {
+    if (!state_->is_constant) {
+      DeleteState();
+    }
+  }
   if (s.state_ == nullptr) {
     state_ = nullptr;
   } else if (s.state_->is_constant) {

diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h
@@ -137,7 +137,13 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
  public:
   // Create a success status.
   constexpr Status() noexcept : state_(NULLPTR) {}
-  ~Status() noexcept { DeleteState(); }
+  ~Status() noexcept {
+    if (ARROW_PREDICT_FALSE(state_ != NULL)) {
+      if (!state_->is_constant) {
+        DeleteState();
+      }
+    }
+  }
 
   Status(StatusCode code, const std::string& msg);
   /// \brief Pluggable constructor for use by sub-systems.  detail cannot be null.
@@ -361,12 +367,11 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
 #endif
 
  private:
-  struct ARROW_EXPORT State {
+  struct State {
     StatusCode code;
     std::string msg;
     std::shared_ptr<StatusDetail> detail;
     bool is_constant = false;
-    ~State();
   };
   // OK status has a `NULL` state_.  Otherwise, `state_` points to
   // a `State` structure containing the error code and message(s)
@@ -375,11 +380,7 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
   void DeleteState() noexcept {
     // ARROW-2400: On certain compilers, splitting off the slow path improves
     // performance significantly.
-    if (ARROW_PREDICT_FALSE(state_ != NULLPTR)) {
-      if (!state_->is_constant) {
-        delete state_;
-      }
-    }
+    delete state_;
   }
   void CopyFrom(const Status& s);
   inline void MoveFrom(Status& s);
@@ -388,7 +389,11 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
 };
 
 void Status::MoveFrom(Status& s) {
-  DeleteState();
+  if (ARROW_PREDICT_FALSE(state_ != NULL)) {
+    if (!state_->is_constant) {
+      DeleteState();
+    }
+  }
   state_ = s.state_;
   s.state_ = NULLPTR;
 }

diff --git a/cpp/src/arrow/status_internal.h b/cpp/src/arrow/status_internal.h
@@ -27,7 +27,8 @@ class StatusConstant {
   StatusConstant(StatusCode code, std::string msg,
                  std::shared_ptr<StatusDetail> detail = nullptr)
       : state_{code, std::move(msg), std::move(detail), /*is_constant=*/true} {
-    ARROW_CHECK_NE(code, StatusCode::OK) << "Cannot construct ok status constant";
+    ARROW_CHECK_NE(code, StatusCode::OK)
+        << "StatusConstant is not intended for use with OK status codes";
   }
 
   operator Status() const {  // NOLINT(runtime/explicit)