-
-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Issue 11308 - Don't use Voldemort types for std.process output #2085
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1259,7 +1259,7 @@ A non-blocking version of $(LREF wait). | |
|
||
If the process associated with $(D pid) has already terminated, | ||
$(D tryWait) has the exact same effect as $(D wait). | ||
In this case, it returns a struct where the $(D terminated) field | ||
In this case, it returns a tuple where the $(D terminated) field | ||
is set to $(D true) and the $(D status) field has the same | ||
interpretation as the return value of $(D wait). | ||
|
||
|
@@ -1273,10 +1273,7 @@ get the exit code, but also to avoid the process becoming a "zombie" | |
when it finally terminates. (See $(LREF wait) for details). | ||
|
||
Returns: | ||
A $(D struct) which contains the fields $(D bool terminated) | ||
and $(D int status). (This will most likely change to become a | ||
$(D std.typecons.Tuple!(bool,"terminated",int,"status")) in the future, | ||
but a compiler bug currently prevents this.) | ||
An $(D std.typecons.Tuple!(bool, "terminated", int, "status")). | ||
|
||
Throws: | ||
$(LREF ProcessException) on failure. | ||
|
@@ -1303,14 +1300,10 @@ by the time we reach the end of the scope. | |
*/ | ||
auto tryWait(Pid pid) @safe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh.. I didn't notice you left There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whether you agree or not, he said that he left it auto so std.typcons is not a global import. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. Somehow my eyes skipped that part when I read @monarchdodra's comment. Sorry about that! I still disagree with this choice, but I'm not going to hold back the PR because of it. After all, it fixes the problem in question here, and it does so without introducing a module-level named struct. |
||
{ | ||
struct TryWaitResult | ||
{ | ||
bool terminated; | ||
int status; | ||
} | ||
import std.typecons : Tuple; | ||
assert(pid !is null, "Called tryWait on a null Pid."); | ||
auto code = pid.performWait(false); | ||
return TryWaitResult(pid._processID == Pid.terminated, code); | ||
return Tuple!(bool, "terminated", int, "status")(pid._processID == Pid.terminated, code); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and if you do what I suggest above, |
||
} | ||
// unittest: This function is tested together with kill() below. | ||
|
||
|
@@ -1946,10 +1939,7 @@ workDir = The working directory for the new process. | |
directory. | ||
|
||
Returns: | ||
A $(D struct) which contains the fields $(D int status) and | ||
$(D string output). (This will most likely change to become a | ||
$(D std.typecons.Tuple!(int,"status",string,"output")) in the future, | ||
but a compiler bug currently prevents this.) | ||
An $(D std.typecons.Tuple!(int, "status", string, "output")). | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comments for |
||
POSIX_specific: | ||
If the process is terminated by a signal, the $(D status) field of | ||
|
@@ -2000,6 +1990,8 @@ private auto executeImpl(alias pipeFunc, Cmd)( | |
size_t maxOutput = size_t.max, | ||
in char[] workDir = null) | ||
{ | ||
import std.typecons : Tuple; | ||
|
||
auto p = pipeFunc(commandLine, Redirect.stdout | Redirect.stderrToStdout, | ||
env, config, workDir); | ||
|
||
|
@@ -2022,8 +2014,7 @@ private auto executeImpl(alias pipeFunc, Cmd)( | |
// Exhaust the stream, if necessary. | ||
foreach (ubyte[] chunk; p.stdout.byChunk(defaultChunkSize)) { } | ||
|
||
struct ProcessOutput { int status; string output; } | ||
return ProcessOutput(wait(p.pid), cast(string) a.data); | ||
return Tuple!(int, "status", string, "output")(wait(p.pid), cast(string) a.data); | ||
} | ||
|
||
unittest | ||
|
@@ -2059,6 +2050,19 @@ unittest | |
assert (r3.output.empty); | ||
} | ||
|
||
unittest | ||
{ | ||
import std.typecons : Tuple; | ||
void foo() //Just test the compilation | ||
{ | ||
auto ret1 = execute(["dummy", "arg"]); | ||
auto ret2 = executeShell("dummy arg"); | ||
static assert(is(typeof(ret1) == typeof(ret2))); | ||
|
||
Tuple!(int, string) ret3 = execute(["dummy", "arg"]); | ||
} | ||
} | ||
|
||
|
||
/// An exception that signals a problem with starting or waiting for a process. | ||
class ProcessException : Exception | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole "Returns" section could be removed, as it no longer provides any additional information beyond what is already in the function signature. Also, further up, in the second paragraph of this doc comment, you could replace
with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the above "struct"/"tuple" thing. Fixed now.