[llvm] 15a6e3c - Support: Make Wait's SecondsToWait be std::optional [NFC]
Matt Arsenault via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 14 06:56:17 PST 2022
Author: Matt Arsenault
Date: 2022-12-14T09:56:10-05:00
New Revision: 15a6e3c636977dc962a415c067182e6d57242116
URL: https://github.com/llvm/llvm-project/commit/15a6e3c636977dc962a415c067182e6d57242116
DIFF: https://github.com/llvm/llvm-project/commit/15a6e3c636977dc962a415c067182e6d57242116.diff
LOG: 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.
Added:
Modified:
llvm/include/llvm/Support/Program.h
llvm/lib/Support/Program.cpp
llvm/lib/Support/Unix/Program.inc
llvm/lib/Support/Windows/Program.inc
llvm/unittests/Support/ProgramTest.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Support/Program.h b/llvm/include/llvm/Support/Program.h
index 5aeb943266d0c..79b038b8ebe7c 100644
--- a/llvm/include/llvm/Support/Program.h
+++ b/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.
diff --git a/llvm/lib/Support/Program.cpp b/llvm/lib/Support/Program.cpp
index dc58872b03c72..1dcd45e2d69e8 100644
--- a/llvm/lib/Support/Program.cpp
+++ b/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;
}
diff --git a/llvm/lib/Support/Unix/Program.inc b/llvm/lib/Support/Unix/Program.inc
index 9dab8e5da08b7..7d451a7eb45a7 100644
--- a/llvm/lib/Support/Unix/Program.inc
+++ b/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;
diff --git a/llvm/lib/Support/Windows/Program.inc b/llvm/lib/Support/Windows/Program.inc
index ae1c59ed84a49..42f4efeb1ff10 100644
--- a/llvm/lib/Support/Windows/Program.inc
+++ b/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");
diff --git a/llvm/unittests/Support/ProgramTest.cpp b/llvm/unittests/Support/ProgramTest.cpp
index 2134b969466a1..0a09278716737 100644
--- a/llvm/unittests/Support/ProgramTest.cpp
+++ b/llvm/unittests/Support/ProgramTest.cpp
@@ -228,11 +228,12 @@ 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;
@@ -240,16 +241,17 @@ TEST_F(ProgramEnvTest, TestExecuteNoWait) {
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);
More information about the llvm-commits
mailing list