Make Subprocess movable

Summary:
Subprocess doesn't have any non-movable members, and its implementation does not take addresses of the object, so I think it's safe. Move makes a bunch of code cleaner (you no longer have to wrap it in `std::unique_ptr` with associated clumsiness).

https://phabricator.fb.com/diffusion/FBCODE/browse/master/folly/Subprocess.h

Test Plan:
- unit test
- Searched for `this` in `Subprocess.{h,cpp}`.
- Inspected member variables: `pid_`, `returnCode_`, `pipes_`
- contbuild

Reviewed By: davejwatson@fb.com

Subscribers: folly-diffs@, yfeldblum, chalfant

FB internal diff: D2079167

Signature: t1:2079167:1432048688:26f96e29310298f47a9a9a7abef22dc863f68942

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 17e997e6854537cb266611a8f453a1513c2e3aab..04866e1dde2ec52079d29b70aa3296a741fa8826 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -50,6 +50,18 @@ constexpr int kChildFailure = 126;
 
 namespace folly {
 
+ProcessReturnCode::ProcessReturnCode(ProcessReturnCode&& p) noexcept
+  : rawStatus_(p.rawStatus_) {
+  p.rawStatus_ = ProcessReturnCode::RV_NOT_STARTED;
+}
+
+ProcessReturnCode& ProcessReturnCode::operator=(ProcessReturnCode&& p)
+    noexcept {
+  rawStatus_ = p.rawStatus_;
+  p.rawStatus_ = ProcessReturnCode::RV_NOT_STARTED;
+  return *this;
+}
+
 ProcessReturnCode::State ProcessReturnCode::state() const {
   if (rawStatus_ == RV_NOT_STARTED) return NOT_STARTED;
   if (rawStatus_ == RV_RUNNING) return RUNNING;

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index cd6afa13c784dc7574fbf802d848dbf47dc0b131..da2b560241fdf80eb9160648d6f5746487b18cc4 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -107,7 +107,6 @@
 
 #include <boost/container/flat_map.hpp>
 #include <boost/operators.hpp>
-#include <boost/noncopyable.hpp>
 
 #include <folly/File.h>
 #include <folly/FileUtil.h>
@@ -133,6 +132,14 @@ class ProcessReturnCode {
     KILLED
   };
 
+  // Trivially copyable
+  ProcessReturnCode(const ProcessReturnCode& p) = default;
+  ProcessReturnCode& operator=(const ProcessReturnCode& p) = default;
+  // Non-default move: In order for Subprocess to be movable, the "moved
+  // out" state must not be "running", or ~Subprocess() will abort.
+  ProcessReturnCode(ProcessReturnCode&& p) noexcept;
+  ProcessReturnCode& operator=(ProcessReturnCode&& p) noexcept;
+
   /**
    * Process state.  One of:
    * NOT_STARTED: process hasn't been started successfully
@@ -227,7 +234,7 @@ class SubprocessSpawnError : public SubprocessError {
 /**
  * Subprocess.
  */
-class Subprocess : private boost::noncopyable {
+class Subprocess {
  public:
   static const int CLOSE = -1;
   static const int PIPE = -2;
@@ -349,6 +356,12 @@ class Subprocess : private boost::noncopyable {
   static Options pipeStdout() { return Options().stdout(PIPE); }
   static Options pipeStderr() { return Options().stderr(PIPE); }
 
+  // Non-copiable, but movable
+  Subprocess(const Subprocess&) = delete;
+  Subprocess& operator=(const Subprocess&) = delete;
+  Subprocess(Subprocess&&) = default;
+  Subprocess& operator=(Subprocess&&) = default;
+
   /**
    * Create a subprocess from the given arguments.  argv[0] must be listed.
    * If not-null, executable must be the actual executable

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 0137bed2aef86189439a3cbf8781dbc40ab7beb0..489e85500aff0b28d9fd21d0b3cec35b96576f11 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -55,6 +55,16 @@ TEST(SimpleSubprocessTest, ExitsWithErrorChecked) {
   EXPECT_THROW(proc.waitChecked(), CalledProcessError);
 }
 
+TEST(SimpleSubprocessTest, MoveSubprocess) {
+  Subprocess old_proc(std::vector<std::string>{ "/bin/true" });
+  EXPECT_TRUE(old_proc.returnCode().running());
+  auto new_proc = std::move(old_proc);
+  EXPECT_TRUE(old_proc.returnCode().notStarted());
+  EXPECT_TRUE(new_proc.returnCode().running());
+  EXPECT_EQ(0, new_proc.wait().exitStatus());
+  // Now old_proc is destroyed, but we don't crash.
+}
+
 #define EXPECT_SPAWN_ERROR(err, errMsg, cmd, ...) \
   do { \
     try { \