Improve the pipe API

include/fmt/os.h

@@ -238,6 +238,7 @@ class buffered_file {
 };
 
 #if FMT_USE_FCNTL
+
 // 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 +252,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 +316,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 +327,15 @@ class FMT_API file {
 #  endif
 };
 
+struct FMT_API 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

@@ -200,6 +200,7 @@ int buffered_file::descriptor() const {
 #  ifdef _WIN32
 using mode_t = int;
 #  endif
+
 constexpr mode_t default_open_mode =
     S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
 
@@ -301,29 +302,6 @@ 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();
-  int fds[2] = {};
-#  ifdef _WIN32
-  // Make the default pipe capacity same as on Linux 2.6.11+.
-  enum { DEFAULT_CAPACITY = 65536 };
-  int result = FMT_POSIX_CALL(pipe(fds, DEFAULT_CAPACITY, _O_BINARY));
-#  else
-  // Don't retry as the pipe function doesn't return EINTR.
-  // http://pubs.opengroup.org/onlinepubs/009696799/functions/pipe.html
-  int result = FMT_POSIX_CALL(pipe(fds));
-#  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.
-  read_end = file(fds[0]);
-  write_end = file(fds[1]);
-}
-
 buffered_file file::fdopen(const char* mode) {
 // Don't retry as fdopen doesn't return EINTR.
 #  if defined(__MINGW32__) && defined(_POSIX_)
@@ -352,6 +330,24 @@ file file::open_windows_file(wcstring_view path, int oflag) {
 }
 #  endif
 
+pipe::pipe() {
+  int fds[2] = {};
+#  ifdef _WIN32
+  // Make the default pipe capacity same as on Linux 2.6.11+.
+  enum { DEFAULT_CAPACITY = 65536 };
+  int result = FMT_POSIX_CALL(pipe(fds, DEFAULT_CAPACITY, _O_BINARY));
+#  else
+  // Don't retry as the pipe function doesn't return EINTR.
+  // http://pubs.opengroup.org/onlinepubs/009696799/functions/pipe.html
+  int result = FMT_POSIX_CALL(pipe(fds));
+#  endif
+  if (result != 0)
+    FMT_THROW(system_error(errno, FMT_STRING("cannot create pipe")));
+  // The following assignments don't throw.
+  read_end = file(fds[0]);
+  write_end = file(fds[1]);
+}
+
 #  if !defined(__MSDOS__)
 long getpagesize() {
 #    ifdef _WIN32

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