Commit 0e5ec48b authored by Alexey Spiridonov's avatar Alexey Spiridonov Committed by Facebook Github Bot

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
parent 6bb90698
...@@ -106,7 +106,6 @@ ...@@ -106,7 +106,6 @@
#include <string> #include <string>
#include <boost/container/flat_map.hpp> #include <boost/container/flat_map.hpp>
#include <boost/operators.hpp>
#include <folly/Exception.h> #include <folly/Exception.h>
#include <folly/File.h> #include <folly/File.h>
...@@ -275,7 +274,7 @@ class Subprocess { ...@@ -275,7 +274,7 @@ class Subprocess {
* the close-on-exec flag is set (fcntl FD_CLOEXEC) and inherited * the close-on-exec flag is set (fcntl FD_CLOEXEC) and inherited
* otherwise. * otherwise.
*/ */
class Options : private boost::orable<Options> { class Options {
friend class Subprocess; friend class Subprocess;
public: public:
Options() {} // E.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328 Options() {} // E.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328
...@@ -421,11 +420,6 @@ class Subprocess { ...@@ -421,11 +420,6 @@ class Subprocess {
} }
#endif #endif
/**
* Helpful way to combine Options.
*/
Options& operator|=(const Options& other);
private: private:
typedef boost::container::flat_map<int, int> FdMap; typedef boost::container::flat_map<int, int> FdMap;
FdMap fdActions_; FdMap fdActions_;
...@@ -891,17 +885,4 @@ class Subprocess { ...@@ -891,17 +885,4 @@ class Subprocess {
std::vector<Pipe> pipes_; 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 } // namespace folly
...@@ -206,8 +206,9 @@ TEST(SimpleSubprocessTest, FdLeakTest) { ...@@ -206,8 +206,9 @@ TEST(SimpleSubprocessTest, FdLeakTest) {
}); });
// Normal execution with pipes // Normal execution with pipes
checkFdLeak([] { checkFdLeak([] {
Subprocess proc("echo foo; echo bar >&2", Subprocess proc(
Subprocess::pipeStdout() | Subprocess::pipeStderr()); "echo foo; echo bar >&2",
Subprocess::Options().pipeStdout().pipeStderr());
auto p = proc.communicate(); auto p = proc.communicate();
EXPECT_EQ("foo\n", p.first); EXPECT_EQ("foo\n", p.first);
EXPECT_EQ("bar\n", p.second); EXPECT_EQ("bar\n", p.second);
...@@ -332,7 +333,7 @@ TEST(CommunicateSubprocessTest, BigWrite) { ...@@ -332,7 +333,7 @@ TEST(CommunicateSubprocessTest, BigWrite) {
data.append(line); data.append(line);
} }
Subprocess proc("wc -l", Subprocess::pipeStdin() | Subprocess::pipeStdout()); Subprocess proc("wc -l", Subprocess::Options().pipeStdin().pipeStdout());
auto p = proc.communicate(data); auto p = proc.communicate(data);
EXPECT_EQ(folly::format("{}\n", numLines).str(), p.first); EXPECT_EQ(folly::format("{}\n", numLines).str(), p.first);
proc.waitChecked(); proc.waitChecked();
...@@ -344,8 +345,7 @@ TEST(CommunicateSubprocessTest, Duplex) { ...@@ -344,8 +345,7 @@ TEST(CommunicateSubprocessTest, Duplex) {
const int bytes = 10 << 20; const int bytes = 10 << 20;
std::string line(bytes, 'x'); std::string line(bytes, 'x');
Subprocess proc("tr a-z A-Z", Subprocess proc("tr a-z A-Z", Subprocess::Options().pipeStdin().pipeStdout());
Subprocess::pipeStdin() | Subprocess::pipeStdout());
auto p = proc.communicate(line); auto p = proc.communicate(line);
EXPECT_EQ(bytes, p.first.size()); EXPECT_EQ(bytes, p.first.size());
EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X')); EXPECT_EQ(std::string::npos, p.first.find_first_not_of('X'));
...@@ -543,8 +543,7 @@ TEST(CommunicateSubprocessTest, TakeOwnershipOfPipes) { ...@@ -543,8 +543,7 @@ TEST(CommunicateSubprocessTest, TakeOwnershipOfPipes) {
std::vector<Subprocess::ChildPipe> pipes; std::vector<Subprocess::ChildPipe> pipes;
{ {
Subprocess proc( Subprocess proc(
"echo $'oh\\nmy\\ncat' | wc -l &", Subprocess::pipeStdout() "echo $'oh\\nmy\\ncat' | wc -l &", Subprocess::Options().pipeStdout());
);
pipes = proc.takeOwnershipOfPipes(); pipes = proc.takeOwnershipOfPipes();
proc.waitChecked(); proc.waitChecked();
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment