[PATCH] D138942: Support: Make Wait's SecondsToWait be Optional [NFC]

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/Support/Program.cpp:35
                         ArrayRef<std::optional<StringRef>> Redirects,
                         unsigned SecondsToWait, unsigned MemoryLimit,
                         std::string *ErrMsg, bool *ExecutionFailed,
----------------
arsenm wrote:
> aganea wrote:
> > `SecondsToWait` here is not completely symmetric with `Wait()` (value 0 here has the meaning of `std::nullopt` in Wait), but maybe it's ok. Do you think it's worth changing this to `std::optional` as well?
> That would expose both the std::nullopt and SecondsToWait == 0 possibilities, which I'm not sure makes any sense here. If you pass a 0, you're expecting to launch a process and have it immediately complete?
The annoying thing is that 0 here means the exact opposite as in `Wait()`.
ExecuteAndWait -> 0 means wait forever.
Wait() -> 0 means don't wait at all.

In that sense, yes if we had `std::option` here and passed 0, that would be (almost) like calling `ExecuteNoWait()`.

Right now we have 6 calling site for `ExecuteNoWait()` across the monorepo, and 47 for  `ExecuteAndWait()`. Maybe we should merge them into one `Execute()`. But let's go ahead with your change and we'll do that as a second step? WDYT?


================
Comment at: llvm/unittests/Support/ProgramTest.cpp:245
+  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(),
+                                  /*Redirects*/ {}, /*MemoryLimit*/ 0, &Error,
                                   &ExecutionFailed);
----------------
/*Redirects=*/ and /*MemoryLimit=*/


================
Comment at: llvm/unittests/Support/ProgramTest.cpp:283
+      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*SecondsToWait=*/ 1,
+                     /*MemoryLimit*/ 0, &Error, &ExecutionFailed);
   ASSERT_EQ(-2, RetCode);
----------------
/*MemoryLimit=*/


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138942/new/

https://reviews.llvm.org/D138942