Improve pipe API

include/fmt/os.h

@@ -238,6 +238,9 @@ class buffered_file {
 };
 
 #if FMT_USE_FCNTL
+
+struct pipe_result;
+
 // A file. Closed file is represented by a file object with descriptor -1.
 // Methods that are not declared with noexcept may throw
 // fmt::system_error in case of failure. Note that some errors such as
@@ -251,6 +254,8 @@ class FMT_API file {
   // Constructs a file object with a given descriptor.
   explicit file(int fd) : fd_(fd) {}
 
+  friend struct pipe;
+
  public:
   // Possible values for the oflag argument to the constructor.
   enum {
@@ -313,11 +318,6 @@ class FMT_API file {
   // necessary.
   void dup2(int fd, std::error_code& ec) noexcept;
 
-  // Creates a pipe setting up read_end and write_end file objects for reading
-  // and writing respectively.
-  // DEPRECATED! Taking files as out parameters is deprecated.
-  static void pipe(file& read_end, file& write_end);
-
   // Creates a buffered_file object associated with this file and detaches
   // this file object from the file.
   auto fdopen(const char* mode) -> buffered_file;
@@ -329,6 +329,15 @@ class FMT_API file {
 #  endif
 };
 
+struct pipe {
+  file read_end;
+  file write_end;
+
+  // Creates a pipe setting up read_end and write_end file objects for reading
+  // and writing respectively.
+  pipe();
+};
+
 // Returns the memory page size.
 auto getpagesize() -> long;
 

src/os.cc

@@ -301,11 +301,7 @@ void file::dup2(int fd, std::error_code& ec) noexcept {
   if (result == -1) ec = std::error_code(errno, std::generic_category());
 }
 
-void file::pipe(file& read_end, file& write_end) {
-  // Close the descriptors first to make sure that assignments don't throw
-  // and there are no leaks.
-  read_end.close();
-  write_end.close();
+pipe::pipe() {
   int fds[2] = {};
 #  ifdef _WIN32
   // Make the default pipe capacity same as on Linux 2.6.11+.
@@ -318,8 +314,7 @@ void file::pipe(file& read_end, file& write_end) {
 #  endif
   if (result != 0)
     FMT_THROW(system_error(errno, FMT_STRING("cannot create pipe")));
-  // The following assignments don't throw because read_fd and write_fd
-  // are closed.
+  // The following assignments don't throw.
   read_end = file(fds[0]);
   write_end = file(fds[1]);
 }

test/gtest-extra-test.cc

@@ -316,27 +316,25 @@ using fmt::buffered_file;
 using fmt::file;
 
 TEST(output_redirect_test, scoped_redirect) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   {
-    buffered_file file(write_end.fdopen("w"));
+    buffered_file file(pipe.write_end.fdopen("w"));
     std::fprintf(file.get(), "[[[");
     {
       output_redirect redir(file.get());
       std::fprintf(file.get(), "censored");
     }
     std::fprintf(file.get(), "]]]");
   }
-  EXPECT_READ(read_end, "[[[]]]");
+  EXPECT_READ(pipe.read_end, "[[[]]]");
 }
 
 // Test that output_redirect handles errors in flush correctly.
 TEST(output_redirect_test, flush_error_in_ctor) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  int write_fd = write_end.descriptor();
-  file write_copy = write_end.dup(write_fd);
-  buffered_file f = write_end.fdopen("w");
+  auto pipe = fmt::pipe();
+  int write_fd = pipe.write_end.descriptor();
+  file write_copy = pipe.write_end.dup(write_fd);
+  buffered_file f = pipe.write_end.fdopen("w");
   // Put a character in a file buffer.
   EXPECT_EQ('x', fputc('x', f.get()));
   FMT_POSIX(close(write_fd));
@@ -360,26 +358,24 @@ TEST(output_redirect_test, dup_error_in_ctor) {
 }
 
 TEST(output_redirect_test, restore_and_read) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  buffered_file file(write_end.fdopen("w"));
+  auto pipe = fmt::pipe();
+  buffered_file file(pipe.write_end.fdopen("w"));
   std::fprintf(file.get(), "[[[");
   output_redirect redir(file.get());
   std::fprintf(file.get(), "censored");
   EXPECT_EQ("censored", redir.restore_and_read());
   EXPECT_EQ("", redir.restore_and_read());
   std::fprintf(file.get(), "]]]");
   file = buffered_file();
-  EXPECT_READ(read_end, "[[[]]]");
+  EXPECT_READ(pipe.read_end, "[[[]]]");
 }
 
 // Test that OutputRedirect handles errors in flush correctly.
 TEST(output_redirect_test, flush_error_in_restore_and_read) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  int write_fd = write_end.descriptor();
-  file write_copy = write_end.dup(write_fd);
-  buffered_file f = write_end.fdopen("w");
+  auto pipe = fmt::pipe();
+  int write_fd = pipe.write_end.descriptor();
+  file write_copy = pipe.write_end.dup(write_fd);
+  buffered_file f = pipe.write_end.fdopen("w");
   output_redirect redir(f.get());
   // Put a character in a file buffer.
   EXPECT_EQ('x', fputc('x', f.get()));
@@ -390,11 +386,10 @@ TEST(output_redirect_test, flush_error_in_restore_and_read) {
 }
 
 TEST(output_redirect_test, error_in_dtor) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  int write_fd = write_end.descriptor();
-  file write_copy = write_end.dup(write_fd);
-  buffered_file f = write_end.fdopen("w");
+  auto pipe = fmt::pipe();
+  int write_fd = pipe.write_end.descriptor();
+  file write_copy = pipe.write_end.dup(write_fd);
+  buffered_file f = pipe.write_end.fdopen("w");
   std::unique_ptr<output_redirect> redir(new output_redirect(f.get()));
   // Put a character in a file buffer.
   EXPECT_EQ('x', fputc('x', f.get()));

test/gtest-extra.cc

@@ -17,10 +17,10 @@ output_redirect::output_redirect(FILE* f, bool flush) : file_(f) {
   // Create a file object referring to the original file.
   original_ = file::dup(fd);
   // Create a pipe.
-  file write_end;
-  file::pipe(read_end_, write_end);
+  auto pipe = fmt::pipe();
+  read_end_ = std::move(pipe.read_end);
   // Connect the passed FILE object to the write end of the pipe.
-  write_end.dup2(fd);
+  pipe.write_end.dup2(fd);
 }
 
 output_redirect::~output_redirect() noexcept {

test/os-test.cc

@@ -104,20 +104,19 @@ TEST(file_test, open_windows_file) {
 
 using fmt::file;
 
-bool isclosed(int fd) {
+auto isclosed(int fd) -> bool {
   char buffer;
   auto result = std::streamsize();
   SUPPRESS_ASSERT(result = FMT_POSIX(read(fd, &buffer, 1)));
   return result == -1 && errno == EBADF;
 }
 
 // Opens a file for reading.
-file open_file() {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  write_end.write(file_content, std::strlen(file_content));
-  write_end.close();
-  return read_end;
+auto open_file() -> file {
+  auto pipe = fmt::pipe();
+  pipe.write_end.write(file_content, std::strlen(file_content));
+  pipe.write_end.close();
+  return std::move(pipe.read_end);
 }
 
 // Attempts to write a string to a file.
@@ -427,11 +426,10 @@ TEST(file_test, read_error) {
 }
 
 TEST(file_test, write) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  write(write_end, "test");
-  write_end.close();
-  EXPECT_READ(read_end, "test");
+  auto pipe = fmt::pipe();
+  write(pipe.write_end, "test");
+  pipe.write_end.close();
+  EXPECT_READ(pipe.read_end, "test");
 }
 
 TEST(file_test, write_error) {
@@ -489,18 +487,16 @@ TEST(file_test, dup2_noexcept_error) {
 }
 
 TEST(file_test, pipe) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  EXPECT_NE(-1, read_end.descriptor());
-  EXPECT_NE(-1, write_end.descriptor());
-  write(write_end, "test");
-  EXPECT_READ(read_end, "test");
+  auto pipe = fmt::pipe();
+  EXPECT_NE(-1, pipe.read_end.descriptor());
+  EXPECT_NE(-1, pipe.write_end.descriptor());
+  write(pipe.write_end, "test");
+  EXPECT_READ(pipe.read_end, "test");
 }
 
 TEST(file_test, fdopen) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  int read_fd = read_end.descriptor();
-  EXPECT_EQ(read_fd, FMT_POSIX(fileno(read_end.fdopen("r").get())));
+  auto pipe = fmt::pipe();
+  int read_fd = pipe.read_end.descriptor();
+  EXPECT_EQ(read_fd, FMT_POSIX(fileno(pipe.read_end.fdopen("r").get())));
 }
 #endif  // FMT_USE_FCNTL

test/posix-mock-test.cc

@@ -225,9 +225,8 @@ TEST(file_test, open_retry) {
 }
 
 TEST(file_test, close_no_retry_in_dtor) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  std::unique_ptr<file> f(new file(std::move(read_end)));
+  auto pipe = fmt::pipe();
+  std::unique_ptr<file> f(new file(std::move(pipe.read_end)));
   int saved_close_count = 0;
   EXPECT_WRITE(
       stderr,
@@ -242,10 +241,9 @@ TEST(file_test, close_no_retry_in_dtor) {
 }
 
 TEST(file_test, close_no_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   close_count = 1;
-  EXPECT_SYSTEM_ERROR(read_end.close(), EINTR, "cannot close file");
+  EXPECT_SYSTEM_ERROR(pipe.read_end.close(), EINTR, "cannot close file");
   EXPECT_EQ(2, close_count);
   close_count = 0;
 }
@@ -283,39 +281,36 @@ TEST(file_test, max_size) {
 }
 
 TEST(file_test, read_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   enum { SIZE = 4 };
-  write_end.write("test", SIZE);
-  write_end.close();
+  pipe.write_end.write("test", SIZE);
+  pipe.write_end.close();
   char buffer[SIZE];
   size_t count = 0;
-  EXPECT_RETRY(count = read_end.read(buffer, SIZE), read,
+  EXPECT_RETRY(count = pipe.read_end.read(buffer, SIZE), read,
                "cannot read from file");
   EXPECT_EQ_POSIX(static_cast<std::streamsize>(SIZE), count);
 }
 
 TEST(file_test, write_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   enum { SIZE = 4 };
   size_t count = 0;
-  EXPECT_RETRY(count = write_end.write("test", SIZE), write,
+  EXPECT_RETRY(count = pipe.write_end.write("test", SIZE), write,
                "cannot write to file");
-  write_end.close();
+  pipe.write_end.close();
 #  ifndef _WIN32
   EXPECT_EQ(static_cast<std::streamsize>(SIZE), count);
   char buffer[SIZE + 1];
-  read_end.read(buffer, SIZE);
+  pipe.read_end.read(buffer, SIZE);
   buffer[SIZE] = '\0';
   EXPECT_STREQ("test", buffer);
 #  endif
 }
 
 #  ifdef _WIN32
 TEST(file_test, convert_read_count) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   char c;
   size_t size = UINT_MAX;
   if (sizeof(unsigned) != sizeof(size_t)) ++size;
@@ -327,8 +322,7 @@ TEST(file_test, convert_read_count) {
 }
 
 TEST(file_test, convert_write_count) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   char c;
   size_t size = UINT_MAX;
   if (sizeof(unsigned) != sizeof(size_t)) ++size;
@@ -372,18 +366,15 @@ TEST(file_test, dup2_no_except_retry) {
 }
 
 TEST(file_test, pipe_no_retry) {
-  file read_end, write_end;
   pipe_count = 1;
-  EXPECT_SYSTEM_ERROR(file::pipe(read_end, write_end), EINTR,
-                      "cannot create pipe");
+  EXPECT_SYSTEM_ERROR(fmt::pipe(), EINTR, "cannot create pipe");
   pipe_count = 0;
 }
 
 TEST(file_test, fdopen_no_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
+  auto pipe = fmt::pipe();
   fdopen_count = 1;
-  EXPECT_SYSTEM_ERROR(read_end.fdopen("r"), EINTR,
+  EXPECT_SYSTEM_ERROR(pipe.read_end.fdopen("r"), EINTR,
                       "cannot associate stream with file descriptor");
   fdopen_count = 0;
 }
@@ -401,9 +392,8 @@ TEST(buffered_file_test, open_retry) {
 }
 
 TEST(buffered_file_test, close_no_retry_in_dtor) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  std::unique_ptr<buffered_file> f(new buffered_file(read_end.fdopen("r")));
+  auto pipe = fmt::pipe();
+  std::unique_ptr<buffered_file> f(new buffered_file(pipe.read_end.fdopen("r")));
   int saved_fclose_count = 0;
   EXPECT_WRITE(
       stderr,
@@ -418,19 +408,17 @@ TEST(buffered_file_test, close_no_retry_in_dtor) {
 }
 
 TEST(buffered_file_test, close_no_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  buffered_file f = read_end.fdopen("r");
+  auto pipe = fmt::pipe();
+  buffered_file f = pipe.read_end.fdopen("r");
   fclose_count = 1;
   EXPECT_SYSTEM_ERROR(f.close(), EINTR, "cannot close file");
   EXPECT_EQ(2, fclose_count);
   fclose_count = 0;
 }
 
 TEST(buffered_file_test, fileno_no_retry) {
-  file read_end, write_end;
-  file::pipe(read_end, write_end);
-  buffered_file f = read_end.fdopen("r");
+  auto pipe = fmt::pipe();
+  buffered_file f = pipe.read_end.fdopen("r");
   fileno_count = 1;
   EXPECT_SYSTEM_ERROR((f.descriptor)(), EINTR, "cannot get file descriptor");
   EXPECT_EQ(2, fileno_count);

test/printf-test.cc

@@ -517,9 +517,8 @@ TEST(printf_test, examples) {
 }
 
 TEST(printf_test, printf_error) {
-  fmt::file read_end, write_end;
-  fmt::file::pipe(read_end, write_end);
-  int result = fmt::fprintf(read_end.fdopen("r").get(), "test");
+  auto pipe = fmt::pipe();
+  int result = fmt::fprintf(pipe.read_end.fdopen("r").get(), "test");
   EXPECT_LT(result, 0);
 }
 #endif