Delete | operator for Subprocess::Options

Summary:
This operator is WRONG. It has not worked correctly for years, e.g. it lacks support for chdir, and several other options.

The operator is not really useful after C++11. Usually, you should just chain setters, e.g. `Subprocess::Options().pipeStdout().pipeStderr()`. If you must repeatedly mutate options in a fixed way, in the C++11 world you can use a lambda instead.

Reviewed By: yfeldblum

Differential Revision: D4862698

fbshipit-source-id: a2d8ace53424b9232e178cf202cf51beb7b59b12

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index 86ca6bfecb80ecc7575b1c859c9d04812d1c8e98..d739288a1c49b73aded1a4679d54009ed534c048 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -106,7 +106,6 @@
 #include <string>
 
 #include <boost/container/flat_map.hpp>
-#include <boost/operators.hpp>
 
 #include <folly/Exception.h>
 #include <folly/File.h>
@@ -275,7 +274,7 @@ class Subprocess {
    * the close-on-exec flag is set (fcntl FD_CLOEXEC) and inherited
    * otherwise.
    */
-  class Options : private boost::orable<Options> {
+  class Options {
     friend class Subprocess;
    public:
     Options() {}  // E.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328
@@ -421,11 +420,6 @@ class Subprocess {
     }
 #endif
 
-    /**
-     * Helpful way to combine Options.
-     */
-    Options& operator|=(const Options& other);
-
    private:
     typedef boost::container::flat_map<int, int> FdMap;
     FdMap fdActions_;
@@ -891,17 +885,4 @@ class Subprocess {
   std::vector<Pipe> pipes_;
 };
 
-inline Subprocess::Options& Subprocess::Options::operator|=(
-    const Subprocess::Options& other) {
-  if (this == &other) return *this;
-  // Replace
-  for (auto& p : other.fdActions_) {
-    fdActions_[p.first] = p.second;
-  }
-  closeOtherFds_ |= other.closeOtherFds_;
-  usePath_ |= other.usePath_;
-  processGroupLeader_ |= other.processGroupLeader_;
-  return *this;
-}
-
 }  // namespace folly

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 8d9f100d269ae154cd8fb599bdda194555b76fce..d9763ae63ecf19316c3164891d8a39057d3aa35a 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -206,8 +206,9 @@ TEST(SimpleSubprocessTest, FdLeakTest) {
   });
   // Normal execution with pipes
   checkFdLeak([] {
-    Subprocess proc("echo foo; echo bar >&2",
-                    Subprocess::pipeStdout() | Subprocess::pipeStderr());
+    Subprocess proc(
+        "echo foo; echo bar >&2",
+        Subprocess::Options().pipeStdout().pipeStderr());
     auto p = proc.communicate();
     EXPECT_EQ("foo\n", p.first);
     EXPECT_EQ("bar\n", p.second);
@@ -332,7 +333,7 @@ TEST(CommunicateSubprocessTest, BigWrite) {
     data.append(line);
   }
 
-  Subprocess proc("wc -l", Subprocess::pipeStdin() | Subprocess::pipeStdout());
+  Subprocess proc("wc -l", Subprocess::Options().pipeStdin().pipeStdout());
   auto p = proc.communicate(data);
   EXPECT_EQ(folly::format("{}\n", numLines).str(), p.first);
   proc.waitChecked();
@@ -344,8 +345,7 @@ TEST(CommunicateSubprocessTest, Duplex) {
   const int bytes = 10 << 20;
   std::string line(bytes, 'x');
 
-  Subprocess proc("tr a-z A-Z",
-                  Subprocess::pipeStdin() | Subprocess::pipeStdout());
+  Subprocess proc("tr a-z A-Z", Subprocess::Options().pipeStdin().pipeStdout());
   auto p = proc.communicate(line);
   EXPECT_EQ(bytes, p.first.size());
   EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X'));
@@ -543,8 +543,7 @@ TEST(CommunicateSubprocessTest, TakeOwnershipOfPipes) {
   std::vector<Subprocess::ChildPipe> pipes;
   {
     Subprocess proc(
-      "echo $'oh\\nmy\\ncat' | wc -l &", Subprocess::pipeStdout()
-    );
+        "echo $'oh\\nmy\\ncat' | wc -l &", Subprocess::Options().pipeStdout());
     pipes = proc.takeOwnershipOfPipes();
     proc.waitChecked();
   }