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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm closed this revision.
arsenm marked an inline comment as done.
arsenm added a comment.

15a6e3c636977dc962a415c067182e6d57242116 <https://reviews.llvm.org/rG15a6e3c636977dc962a415c067182e6d57242116>



================
Comment at: llvm/lib/Support/Program.cpp:35
                         ArrayRef<std::optional<StringRef>> Redirects,
                         unsigned SecondsToWait, unsigned MemoryLimit,
                         std::string *ErrMsg, bool *ExecutionFailed,
----------------
aganea wrote:
> 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?
They could be merged, though it removes the "simple" option


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

https://reviews.llvm.org/D138942