Further refactoring of io_buffer_t

Previously we sometimes wanted to access an io_buffer_t to append to it
directly, but that's no longer true; all we really care about is its
separated_buffer_t. Make io_bufferfill_t::finish return the
separated_buffer directly, simplifying call sites. No user visible changes
expected here.

src/builtin_eval.cpp

@@ -74,12 +74,12 @@ maybe_t<int> builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **arg
     // deallocate to close.
     ios.clear();
     if (stdout_fill) {
-        std::shared_ptr<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
+        separated_buffer_t output = io_bufferfill_t::finish(std::move(stdout_fill));
-        streams.out.append_narrow_buffer(output->take_buffer());
+        streams.out.append_narrow_buffer(std::move(output));
     }
     if (stderr_fill) {
-        std::shared_ptr<io_buffer_t> errput = io_bufferfill_t::finish(std::move(stderr_fill));
+        separated_buffer_t errput = io_bufferfill_t::finish(std::move(stderr_fill));
-        streams.err.append_narrow_buffer(errput->take_buffer());
+        streams.err.append_narrow_buffer(std::move(errput));
     }
     return status;
 }

src/exec.cpp

@@ -690,8 +690,8 @@ static launch_result_t exec_block_or_func_process(parser_t &parser, const std::s
         // Remove our write pipe and forget it. This may close the pipe, unless another thread has
         // claimed it (background write) or another process has inherited it.
         io_chain.remove(block_output_bufferfill);
-        auto block_output_buffer = io_bufferfill_t::finish(std::move(block_output_bufferfill));
+        buffer_contents =
-        buffer_contents = block_output_buffer->take_buffer().newline_serialized();
+            io_bufferfill_t::finish(std::move(block_output_bufferfill)).newline_serialized();
     }
 
     run_internal_process_or_short_circuit(parser, j, p, std::move(buffer_contents),
@@ -1110,7 +1110,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser,
         return STATUS_CMD_ERROR;
     }
     eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, job_group, block_type_t::subst);
-    separated_buffer_t buffer = io_bufferfill_t::finish(std::move(bufferfill))->take_buffer();
+    separated_buffer_t buffer = io_bufferfill_t::finish(std::move(bufferfill));
     if (buffer.discarded()) {
         *break_expand = true;
         return STATUS_READ_TOO_MUCH;

src/fish_tests.cpp

@@ -1302,7 +1302,7 @@ static void test_1_cancellation(const wchar_t *src) {
         pthread_kill(thread, SIGINT);
     });
     eval_res_t res = parser_t::principal_parser().eval(src, io_chain_t{filler});
-    auto buffer = io_bufferfill_t::finish(std::move(filler))->take_buffer();
+    separated_buffer_t buffer = io_bufferfill_t::finish(std::move(filler));
     if (buffer.size() != 0) {
         err(L"Expected 0 bytes in out_buff, but instead found %lu bytes, for command %ls\n",
             buffer.size(), src);

src/io.cpp

@@ -136,7 +136,7 @@ void io_buffer_t::begin_filling(autoclose_fd_t fd) {
     this->item_id_ = fd_monitor().add(std::move(item));
 }
 
-void io_buffer_t::complete_background_fillthread() {
+separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer() {
     // Mark that our fillthread is done, then wake it up.
     ASSERT_IS_MAIN_THREAD();
     assert(fillthread_running() && "Should have a fillthread");
@@ -147,7 +147,13 @@ void io_buffer_t::complete_background_fillthread() {
     // Wait for the fillthread to fulfill its promise, and then clear the future so we know we no
     // longer have one.
     fillthread_waiter_.wait();
-    fillthread_waiter_ = {};
+    fillthread_waiter_ = std::future<void>{};
+
+    // Return our buffer, transferring ownership.
+    auto locked_buff = buffer_.acquire();
+    separated_buffer_t result = std::move(*locked_buff);
+    locked_buff->clear();
+    return result;
 }
 
 shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const fd_set_t &conflicts, size_t buffer_limit,
@@ -173,7 +179,7 @@ shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const fd_set_t &conflicts, s
     return std::make_shared<io_bufferfill_t>(target, std::move(pipes->write), buffer);
 }
 
-std::shared_ptr<io_buffer_t> io_bufferfill_t::finish(std::shared_ptr<io_bufferfill_t> &&filler) {
+separated_buffer_t io_bufferfill_t::finish(std::shared_ptr<io_bufferfill_t> &&filler) {
     // The io filler is passed in. This typically holds the only instance of the write side of the
     // pipe used by the buffer's fillthread (except for that side held by other processes). Get the
     // buffer out of the bufferfill and clear the shared_ptr; this will typically widow the pipe.
@@ -181,8 +187,7 @@ std::shared_ptr<io_buffer_t> io_bufferfill_t::finish(std::shared_ptr<io_bufferfi
     assert(filler && "Null pointer in finish");
     auto buffer = filler->buffer();
     filler.reset();
-    buffer->complete_background_fillthread();
+    return buffer->complete_background_fillthread_and_take_buffer();
-    return buffer;
 }
 
 io_buffer_t::~io_buffer_t() {

src/io.h

@@ -75,7 +75,7 @@ class separated_buffer_t {
     void operator=(const separated_buffer_t &) = delete;
 
     /// We may be moved.
-    /// Note this leaves the moved-from value in a bogus state, until clear() is called on it.
+    /// Note this leaves the moved-from value in a bogus state until clear() is called on it.
     separated_buffer_t(separated_buffer_t &&rhs) = default;
     separated_buffer_t &operator=(separated_buffer_t &&) = default;
 
@@ -303,7 +303,7 @@ class io_bufferfill_t final : public io_data_t {
 
     /// Reset the receiver (possibly closing the write end of the pipe), and complete the fillthread
     /// of the buffer. \return the buffer.
-    static std::shared_ptr<io_buffer_t> finish(std::shared_ptr<io_bufferfill_t> &&filler);
+    static separated_buffer_t finish(std::shared_ptr<io_bufferfill_t> &&filler);
 };
 
 class output_stream_t;
@@ -316,16 +316,6 @@ public:
 
     ~io_buffer_t();
 
-    /// Take the underlying buffer, transferring ownership to the caller.
-    /// This should only be called after the fillthread operation is complete.
-    separated_buffer_t take_buffer() {
-        assert(!fillthread_running() && "Cannot access buffer during background fill");
-        auto locked_buff = buffer_.acquire();
-        separated_buffer_t result = std::move(*locked_buff);
-        locked_buff->clear();
-        return result;
-    }
-
     /// Append a string to the buffer.
     void append(std::string &&str, separation_type_t type = separation_type_t::inferred) {
         buffer_.acquire()->append(std::move(str), type);
@@ -343,8 +333,8 @@ public:
     /// Begin the fill operation, reading from the given fd in the background.
     void begin_filling(autoclose_fd_t readfd);
 
-    /// End the background fillthread operation.
+    /// End the background fillthread operation, and return the buffer, transferring ownership.
-    void complete_background_fillthread();
+    separated_buffer_t complete_background_fillthread_and_take_buffer();
 
     /// Helper to return whether the fillthread is running.
     bool fillthread_running() const { return fillthread_waiter_.valid(); }