ARROW-8367: [C++] Deprecate Buffer::FromString(..., MemoryPool*)

Closes #6869 from pitrou/ARROW-8367-deprecate-buffer-fromstring-pool

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>

cpp/src/arrow/buffer.cc

@@ -67,21 +67,22 @@ bool Buffer::Equals(const Buffer& other) const {
                              !memcmp(data_, other.data_, static_cast<size_t>(size_))));
 }
 
-Result<std::shared_ptr<Buffer>> Buffer::FromString(const std::string& data,
-                                                   MemoryPool* pool) {
+static Status BufferFromString(const std::string& data, MemoryPool* pool,
+                               std::shared_ptr<Buffer>* out) {
   auto size = static_cast<int64_t>(data.size());
   ARROW_ASSIGN_OR_RAISE(auto buf, AllocateBuffer(size, pool));
   std::copy(data.c_str(), data.c_str() + size, buf->mutable_data());
-  return std::move(buf);
+  *out = std::move(buf);
+  return Status::OK();
 }
 
 Status Buffer::FromString(const std::string& data, MemoryPool* pool,
                           std::shared_ptr<Buffer>* out) {
-  return FromString(data, pool).Value(out);
+  return BufferFromString(data, pool, out);
 }
 
 Status Buffer::FromString(const std::string& data, std::shared_ptr<Buffer>* out) {
-  return FromString(data).Value(out);
+  return BufferFromString(data, default_memory_pool(), out);
 }
 
 std::string Buffer::ToString() const {
@@ -127,7 +128,7 @@ Result<std::shared_ptr<Buffer>> Buffer::ViewOrCopy(
 
 class StlStringBuffer : public Buffer {
  public:
-  explicit StlStringBuffer(std::string&& data)
+  explicit StlStringBuffer(std::string data)
       : Buffer(nullptr, 0), input_(std::move(data)) {
     data_ = reinterpret_cast<const uint8_t*>(input_.c_str());
     size_ = static_cast<int64_t>(input_.size());
@@ -138,7 +139,7 @@ class StlStringBuffer : public Buffer {
   std::string input_;
 };
 
-std::shared_ptr<Buffer> Buffer::FromString(std::string&& data) {
+std::shared_ptr<Buffer> Buffer::FromString(std::string data) {
   return std::make_shared<StlStringBuffer>(std::move(data));
 }
 

cpp/src/arrow/buffer.h

@@ -145,28 +145,20 @@ class ARROW_EXPORT Buffer {
     }
   }
 
-  /// \brief Construct a new buffer that owns its memory from a std::string
-  ///
-  /// The string data is copied into the newly-allocated buffer memory.
+  /// \brief Construct an immutable buffer that takes ownership of the contents
+  /// of an std::string (without copying it).
   ///
-  /// \param[in] data a std::string object
-  /// \param[in] pool a memory pool
-  static Result<std::shared_ptr<Buffer>> FromString(
-      const std::string& data, MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT);
+  /// \param[in] data a string to own
+  /// \return a new Buffer instance
+  static std::shared_ptr<Buffer> FromString(std::string data);
 
-  ARROW_DEPRECATED("Use Result-returning version")
+  ARROW_DEPRECATED("Use Buffer-returning version")
   static Status FromString(const std::string& data, MemoryPool* pool,
                            std::shared_ptr<Buffer>* out);
 
-  ARROW_DEPRECATED("Use Result-returning version")
+  ARROW_DEPRECATED("Use Buffer-returning version")
   static Status FromString(const std::string& data, std::shared_ptr<Buffer>* out);
 
-  /// \brief Construct an immutable buffer that takes ownership of the contents
-  /// of an std::string
-  /// \param[in] data an rvalue-reference of a string
-  /// \return a new Buffer instance
-  static std::shared_ptr<Buffer> FromString(std::string&& data);
-
   /// \brief Create buffer referencing typed memory with some length without
   /// copying
   /// \param[in] data the typed memory as C array

cpp/src/arrow/buffer_test.cc

@@ -366,7 +366,7 @@ TEST(TestBuffer, FromStdStringWithMemory) {
 
   {
     std::string temp = "hello, world";
-    ASSERT_OK_AND_ASSIGN(buf, Buffer::FromString(temp));
+    buf = Buffer::FromString(temp);
     AssertIsCPUBuffer(*buf);
     ASSERT_EQ(0, memcmp(buf->data(), temp.c_str(), temp.size()));
     ASSERT_EQ(static_cast<int64_t>(temp.size()), buf->size());

cpp/src/arrow/filesystem/mockfs.cc

@@ -371,8 +371,7 @@ class MockFileSystem::Impl {
     if (!entry->is_file()) {
       return NotAFile(path);
     }
-    ARROW_ASSIGN_OR_RAISE(auto buffer, Buffer::FromString(entry->as_file().data));
-    return std::make_shared<io::BufferReader>(buffer);
+    return std::make_shared<io::BufferReader>(Buffer::FromString(entry->as_file().data));
   }
 };
 

cpp/src/arrow/flight/flight_test.cc

@@ -390,7 +390,7 @@ class TestFlightClient : public ::testing::Test {
 class AuthTestServer : public FlightServerBase {
   Status DoAction(const ServerCallContext& context, const Action& action,
                   std::unique_ptr<ResultStream>* result) override {
-    ARROW_ASSIGN_OR_RAISE(auto buf, Buffer::FromString(context.peer_identity()));
+    auto buf = Buffer::FromString(context.peer_identity());
     *result = std::unique_ptr<ResultStream>(new SimpleResultStream({Result{buf}}));
     return Status::OK();
   }
@@ -717,8 +717,7 @@ class ReportContextTestServer : public FlightServerBase {
     if (middleware == nullptr || middleware->name() != "TracingServerMiddleware") {
       buf = Buffer::FromString("");
     } else {
-      ARROW_ASSIGN_OR_RAISE(
-          buf, Buffer::FromString(((const TracingServerMiddleware*)middleware)->span_id));
+      buf = Buffer::FromString(((const TracingServerMiddleware*)middleware)->span_id);
     }
     *result = std::unique_ptr<ResultStream>(new SimpleResultStream({Result{buf}}));
     return Status::OK();
@@ -1003,7 +1002,7 @@ TEST_F(TestFlightClient, DoAction) {
   action.type = "action1";
 
   const std::string action1_value = "action1-content";
-  action.body = *Buffer::FromString(action1_value);
+  action.body = Buffer::FromString(action1_value);
   ASSERT_OK(client_->DoAction(action, &stream));
 
   for (int i = 0; i < 3; ++i) {

cpp/src/arrow/flight/internal.cc

@@ -305,7 +305,8 @@ Status ToProto(const ActionType& type, pb::ActionType* pb_type) {
 
 Status FromProto(const pb::Action& pb_action, Action* action) {
   action->type = pb_action.type();
-  return Buffer::FromString(pb_action.body()).Value(&action->body);
+  action->body = Buffer::FromString(pb_action.body());
+  return Status::OK();
 }
 
 Status ToProto(const Action& action, pb::Action* pb_action) {
@@ -321,7 +322,8 @@ Status ToProto(const Action& action, pb::Action* pb_action) {
 Status FromProto(const pb::Result& pb_result, Result* result) {
   // ARROW-3250; can avoid copy. Can also write custom deserializer if it
   // becomes an issue
-  return Buffer::FromString(pb_result.body()).Value(&result->body);
+  result->body = Buffer::FromString(pb_result.body());
+  return Status::OK();
 }
 
 Status ToProto(const Result& result, pb::Result* pb_result) {