Support: Make Wait's SecondsToWait be std::optional [NFC]

I found the interaction between SecondsToWait and
WaitUntilChildTerminates confusing. Rather than have a boolean to
ignore the value of SecondsToWait, combine these into one Optional
parameter.

Author: arsenm

llvm/include/llvm/Support/Program.h

@@ -205,22 +205,22 @@ namespace sys {
   /// \li 0 if the child process has not changed state.
   /// \note Users of this function should always check the ReturnCode member of
   /// the \see ProcessInfo returned from this function.
-  ProcessInfo Wait(
-      const ProcessInfo &PI,  ///< The child process that should be waited on.
-      unsigned SecondsToWait, ///< If non-zero, this specifies the amount of
-      ///< time to wait for the child process to exit. If the time expires, the
-      ///< child is killed and this function returns. If zero, this function
-      ///< will perform a non-blocking wait on the child process.
-      bool WaitUntilTerminates, ///< If true, ignores \p SecondsToWait and waits
-      ///< until child has terminated.
-      std::string *ErrMsg = nullptr, ///< If non-zero, provides a pointer to a
-      ///< string instance in which error messages will be returned. If the
-      ///< string is non-empty upon return an error occurred while invoking the
-      ///< program.
-      std::optional<ProcessStatistics> *ProcStat =
-          nullptr ///< If non-zero, provides
-      /// a pointer to a structure in which process execution statistics will be
-      /// stored.
+  ProcessInfo
+  Wait(const ProcessInfo &PI, ///< The child process that should be waited on.
+       std::optional<unsigned> SecondsToWait, ///< If std::nullopt, waits until
+       ///< child has terminated.
+       ///< If a value, this specifies the amount of time to wait for the child
+       ///< process to exit. If the time expires, the child is killed and this
+       ///< function returns. If zero, this function will perform a non-blocking
+       ///< wait on the child process.
+       std::string *ErrMsg = nullptr, ///< If non-zero, provides a pointer to a
+       ///< string instance in which error messages will be returned. If the
+       ///< string is non-empty upon return an error occurred while invoking the
+       ///< program.
+       std::optional<ProcessStatistics> *ProcStat =
+           nullptr ///< If non-zero, provides
+       /// a pointer to a structure in which process execution statistics will
+       /// be stored.
   );
 
   /// Print a command argument, and optionally quote it.

llvm/lib/Support/Program.cpp

@@ -42,9 +42,9 @@ int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
               AffinityMask)) {
     if (ExecutionFailed)
       *ExecutionFailed = false;
-    ProcessInfo Result =
-        Wait(PI, SecondsToWait, /*WaitUntilTerminates=*/SecondsToWait == 0,
-             ErrMsg, ProcStat);
+    ProcessInfo Result = Wait(
+        PI, SecondsToWait == 0 ? std::nullopt : std::optional(SecondsToWait),
+        ErrMsg, ProcStat);
     return Result.ReturnCode;
   }
 

llvm/lib/Support/Unix/Program.inc

@@ -384,17 +384,19 @@ pid_t(llvm::sys::wait4)(pid_t pid, int *status, int options,
 }
 #endif
 
-ProcessInfo llvm::sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
-                            bool WaitUntilTerminates, std::string *ErrMsg,
+ProcessInfo llvm::sys::Wait(const ProcessInfo &PI,
+                            std::optional<unsigned> SecondsToWait,
+                            std::string *ErrMsg,
                             std::optional<ProcessStatistics> *ProcStat) {
   struct sigaction Act, Old;
   assert(PI.Pid && "invalid pid to wait on, process not started?");
 
   int WaitPidOptions = 0;
   pid_t ChildPid = PI.Pid;
-  if (WaitUntilTerminates) {
-    SecondsToWait = 0;
-  } else if (SecondsToWait) {
+  bool WaitUntilTerminates = false;
+  if (!SecondsToWait) {
+    WaitUntilTerminates = true;
+  } else if (*SecondsToWait != 0) {
     // Install a timeout handler.  The handler itself does nothing, but the
     // simple fact of having a handler at all causes the wait below to return
     // with EINTR, unlike if we used SIG_IGN.
@@ -403,9 +405,11 @@ ProcessInfo llvm::sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
     sigemptyset(&Act.sa_mask);
     sigaction(SIGALRM, &Act, &Old);
     // FIXME The alarm signal may be delivered to another thread.
-    alarm(SecondsToWait);
-  } else if (SecondsToWait == 0)
+
+    alarm(*SecondsToWait);
+  } else {
     WaitPidOptions = WNOHANG;
+  }
 
   // Parent process: Wait for the child process to terminate.
   int status;

llvm/lib/Support/Windows/Program.inc

@@ -408,24 +408,21 @@ ErrorOr<std::wstring> sys::flattenWindowsCommandLine(ArrayRef<StringRef> Args) {
   return std::wstring(CommandUtf16.begin(), CommandUtf16.end());
 }
 
-ProcessInfo sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
-                      bool WaitUntilChildTerminates, std::string *ErrMsg,
+ProcessInfo sys::Wait(const ProcessInfo &PI,
+                      std::optional<unsigned> SecondsToWait,
+                      std::string *ErrMsg,
                       std::optional<ProcessStatistics> *ProcStat) {
   assert(PI.Pid && "invalid pid to wait on, process not started?");
   assert((PI.Process && PI.Process != INVALID_HANDLE_VALUE) &&
          "invalid process handle to wait on, process not started?");
-  DWORD milliSecondsToWait = 0;
-  if (WaitUntilChildTerminates)
-    milliSecondsToWait = INFINITE;
-  else if (SecondsToWait > 0)
-    milliSecondsToWait = SecondsToWait * 1000;
+  DWORD milliSecondsToWait = SecondsToWait ? *SecondsToWait * 1000 : INFINITE;
 
   ProcessInfo WaitResult = PI;
   if (ProcStat)
     ProcStat->reset();
   DWORD WaitStatus = WaitForSingleObject(PI.Process, milliSecondsToWait);
   if (WaitStatus == WAIT_TIMEOUT) {
-    if (SecondsToWait) {
+    if (*SecondsToWait > 0) {
       if (!TerminateProcess(PI.Process, 1)) {
         if (ErrMsg)
           MakeErrMsg(ErrMsg, "Failed to terminate timed-out program");

llvm/unittests/Support/ProgramTest.cpp

@@ -228,28 +228,30 @@ TEST_F(ProgramEnvTest, TestExecuteNoWait) {
 
   unsigned LoopCount = 0;
 
-  // Test that Wait() with WaitUntilTerminates=true works. In this case,
+  // Test that Wait() with SecondsToWait=std::nullopt works. In this case,
   // LoopCount should only be incremented once.
   while (true) {
     ++LoopCount;
-    ProcessInfo WaitResult = llvm::sys::Wait(PI1, 0, true, &Error);
+    ProcessInfo WaitResult =
+        llvm::sys::Wait(PI1, /*SecondsToWait=*/std::nullopt, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI1.Pid)
       break;
   }
 
   EXPECT_EQ(LoopCount, 1u) << "LoopCount should be 1";
 
-  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
+  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(),
+                                  /*Redirects*/ {}, /*MemoryLimit*/ 0, &Error,
                                   &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI2.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
   // Test that Wait() with SecondsToWait=0 performs a non-blocking wait. In this
-  // cse, LoopCount should be greater than 1 (more than one increment occurs).
+  // case, LoopCount should be greater than 1 (more than one increment occurs).
   while (true) {
     ++LoopCount;
-    ProcessInfo WaitResult = llvm::sys::Wait(PI2, 0, false, &Error);
+    ProcessInfo WaitResult = llvm::sys::Wait(PI2, /*SecondsToWait=*/0, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI2.Pid)
       break;
@@ -277,8 +279,8 @@ TEST_F(ProgramEnvTest, TestExecuteAndWaitTimeout) {
   std::string Error;
   bool ExecutionFailed;
   int RetCode =
-      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*secondsToWait=*/1, 0,
-                     &Error, &ExecutionFailed);
+      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*SecondsToWait=*/1,
+                     /*MemoryLimit*/ 0, &Error, &ExecutionFailed);
   ASSERT_EQ(-2, RetCode);
 }
 
@@ -423,7 +425,7 @@ TEST_F(ProgramEnvTest, TestLockFile) {
   std::this_thread::sleep_for(std::chrono::milliseconds(100));
 
   ASSERT_NO_ERROR(fs::unlockFile(FD1));
-  ProcessInfo WaitResult = llvm::sys::Wait(PI2, 5 /* seconds */, true, &Error);
+  ProcessInfo WaitResult = llvm::sys::Wait(PI2, /*SecondsToWait=*/5, &Error);
   ASSERT_TRUE(Error.empty());
   ASSERT_EQ(0, WaitResult.ReturnCode);
   ASSERT_EQ(WaitResult.Pid, PI2.Pid);