GH-44491: [C++] StatusConstant- cheaply copied const Status

cpp/src/arrow/result.cc

@@ -19,18 +19,21 @@
 
 #include <string>
 
+#include "arrow/status_internal.h"
 #include "arrow/util/logging.h"
 
-namespace arrow {
-
-namespace internal {
+namespace arrow::internal {
 
 void DieWithMessage(const std::string& msg) { ARROW_LOG(FATAL) << msg; }
 
 void InvalidValueOrDie(const Status& st) {
   DieWithMessage(std::string("ValueOrDie called on an error: ") + st.ToString());
 }
 
-}  // namespace internal
+Status UninitializedResult() {
+  static StatusConstant uninitialized_result{StatusCode::UnknownError,
+                                             "Uninitialized Result<T>"};
+  return uninitialized_result;
+}
 
-}  // namespace arrow
+}  // namespace arrow::internal

cpp/src/arrow/result.h

@@ -39,6 +39,8 @@ ARROW_EXPORT void DieWithMessage(const std::string& msg);
 
 ARROW_EXPORT void InvalidValueOrDie(const Status& st);
 
+ARROW_EXPORT Status UninitializedResult();
+
 }  // namespace internal
 
 /// A class for representing either a usable value, or an error.
@@ -112,7 +114,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
   /// an empty vector, it will actually invoke the default constructor of
   /// Result.
   explicit Result() noexcept  // NOLINT(runtime/explicit)
-      : status_(Status::UnknownError("Uninitialized Result<T>")) {}
+      : status_(internal::UninitializedResult()) {}
 
   ~Result() noexcept { Destroy(); }
 
@@ -294,7 +296,18 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
   ///
   /// \return The stored non-OK status object, or an OK status if this object
   ///         has a value.
-  constexpr const Status& status() const { return status_; }
+  constexpr const Status& status() const& { return status_; }
+
+  /// Gets the stored status object, or an OK status if a `T` value is stored.
+  ///
+  /// \return The stored non-OK status object, or an OK status if this object
+  ///         has a value.
+  Status status() && {
+    if (ARROW_PREDICT_TRUE(ok())) return Status::OK();
+    Status st = std::move(status_);
+    status_ = internal::UninitializedResult();
+    return st;
+  }
 
   /// Gets the stored `T` value.
   ///

cpp/src/arrow/status.cc

@@ -15,7 +15,9 @@
 #include <cassert>
 #include <cstdlib>
 #include <iostream>
-#include <sstream>
+#ifdef ARROW_EXTRA_ERROR_CONTEXT
+#  include <sstream>
+#endif
 
 #include "arrow/util/logging.h"
 
@@ -26,18 +28,19 @@ Status::Status(StatusCode code, const std::string& msg)
 
 Status::Status(StatusCode code, std::string msg, std::shared_ptr<StatusDetail> detail) {
   ARROW_CHECK_NE(code, StatusCode::OK) << "Cannot construct ok status with message";
-  state_ = new State;
-  state_->code = code;
-  state_->msg = std::move(msg);
-  if (detail != nullptr) {
-    state_->detail = std::move(detail);
-  }
+  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) {
-  delete state_;
+  DeleteState();
   if (s.state_ == nullptr) {
     state_ = nullptr;
+  } else if (s.state_->is_constant) {
+    state_ = s.state_;
   } else {
     state_ = new State(*s.state_);
   }
@@ -160,7 +163,11 @@ void Status::AddContextLine(const char* filename, int line, const char* expr) {
   ARROW_CHECK(!ok()) << "Cannot add context line to ok status";
   std::stringstream ss;
   ss << "\n" << filename << ":" << line << "  " << expr;
-  state_->msg += ss.str();
+  if (state_->is_constant) {
+    // We can't add context lines to a StatusConstant's state, so copy it now
+    state_ = new State{code(), message(), detail()};
+  }
+  const_cast<State*>(state_)->msg += ss.str();
 }
 #endif
 

cpp/src/arrow/status.h

@@ -82,6 +82,9 @@
 #endif
 
 namespace arrow {
+namespace internal {
+class StatusConstant;
+}
 
 enum class StatusCode : char {
   OK = 0,
@@ -134,13 +137,7 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
  public:
   // Create a success status.
   constexpr Status() noexcept : state_(NULLPTR) {}
-  ~Status() noexcept {
-    // ARROW-2400: On certain compilers, splitting off the slow path improves
-    // performance significantly.
-    if (ARROW_PREDICT_FALSE(state_ != NULL)) {
-      DeleteState();
-    }
-  }
+  ~Status() noexcept { DeleteState(); }
 
   Status(StatusCode code, const std::string& msg);
   /// \brief Pluggable constructor for use by sub-systems.  detail cannot be null.
@@ -364,31 +361,39 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
 #endif
 
  private:
-  struct State {
+  struct ARROW_EXPORT 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)
-  State* state_;
+  const State* state_;
 
-  void DeleteState() {
-    delete state_;
-    state_ = NULLPTR;
+  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_;
+      }
+    }
   }
   void CopyFrom(const Status& s);
   inline void MoveFrom(Status& s);
+
+  friend class internal::StatusConstant;
 };
 
 void Status::MoveFrom(Status& s) {
-  delete state_;
+  DeleteState();
   state_ = s.state_;
   s.state_ = NULLPTR;
 }
 
-Status::Status(const Status& s)
-    : state_((s.state_ == NULLPTR) ? NULLPTR : new State(*s.state_)) {}
+Status::Status(const Status& s) : state_{NULLPTR} { CopyFrom(s); }
 
 Status& Status::operator=(const Status& s) {
   // The following condition catches both aliasing (when this == &s),

cpp/src/arrow/status_internal.h

@@ -0,0 +1,43 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "arrow/status.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::internal {
+
+class StatusConstant {
+ public:
+  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";
+  }
+
+  operator Status() const {  // NOLINT(runtime/explicit)
+    Status st;
+    st.state_ = &state_;
+    return st;
+  }
+
+ private:
+  Status::State state_;
+};
+
+}  // namespace arrow::internal

cpp/src/arrow/status_test.cc

@@ -21,6 +21,7 @@
 #include <gtest/gtest.h>
 
 #include "arrow/status.h"
+#include "arrow/status_internal.h"
 #include "arrow/testing/gtest_util.h"
 #include "arrow/testing/matchers.h"
 
@@ -76,6 +77,18 @@ TEST(StatusTest, TestCoverageWarnNotOK) {
   ARROW_WARN_NOT_OK(Status::Invalid("invalid"), "Expected warning");
 }
 
+TEST(StatusTest, StatusConstant) {
+  internal::StatusConstant constant{StatusCode::Invalid, "default error"};
+  Status st = constant;
+
+  ASSERT_EQ(st.code(), StatusCode::Invalid);
+  ASSERT_EQ(st.message(), "default error");
+  ASSERT_EQ(st.detail(), nullptr);
+
+  Status copy = st;
+  ASSERT_EQ(&st.message(), &copy.message());
+}
+
 TEST(StatusTest, AndStatus) {
   Status a = Status::OK();
   Status b = Status::OK();