Hi Andrey,
Sorry to be slow in getting back to this.
Thanks for the update; did you notice that Windows has a separate
implementation of
ProcessImpl and would also need a similar, though not identical
toString() implementation?
[jdk/src/java.base/windows/classes/java/lang/ProcessImpl.java ]
Windows has a slightly different implementation of the internal state
with respect to knowing if it has exited.
See exitValue() and the native getExitCodeProcess() method.
On 7/24/2016 7:42 AM, Andrey Dyachkov wrote:
Hi Roger,
Thank you for reviewing it! I've prepared another version.
diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
b/src/java.base/unix/classes/java/lang/ProcessImpl.java
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -630,6 +630,21 @@
return !hasExited;
}
+ /**
+ * The {@code toString} method for class {@code ProcessImpl}
+ * returns a string consisting of the native process ID of the
process,
+ * exit code of the process and the status of the process.
The javadoc for Process uses the terminology "exit value" or "exit
status"; the usage should be consistent.
+ *
+ * @return a string representation of the object.
+ */
+ @Override
+ public String toString() {
+ return new StringBuilder("ProcessImpl{pid=").append(pid)
The implementation class should be hidden, use "Process".
Other toString methods use '[]' to enclose the aggregate of the values.
+ .append(", exitcode=").append(exitcode)
'exitcode=' -> "exitValue" to be consistent with the exitValue method.
This will need an alternate value if the process has not exited, perhaps
"not exited"
+ .append(", hasExited=").append(hasExited)
This isn't needed with the above 'not exited' string.
+ .append("}").toString();
'}' -> ']'
Also, I think this would be viewed more as an enhancement than a bug and
we'll need
to request an extension[1][2] for it when the code is complete.
Thanks, Roger
[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004443.html
[2] http://openjdk.java.net/projects/jdk9/fc-extension-process
+ }
+
private static native void init();
static {
On Mon, 9 May 2016 at 16:50 Roger Riggs <roger.ri...@oracle.com
<mailto:roger.ri...@oracle.com>> wrote:
Hi Andrey,
Since toString is a public method it needs to provide complete javadoc
specification
as providing values for debugging information. ToString methods
typically directly
reflect the state of the fields of the object.
Why should the exitValue have the value of the pid if the process
is not
alive?
Also, the if toString is providing the exitValue then it should be
identified as "exitValue", not exitCode.
the isAlive value should be identified as "isAlive".
The implementation class should not be exposed. (remove
this.getClass().getSimpleName()).
The performance of what seems like a simple toString method is not
going
to be great
because of the native calls necessary to determine if the process is
alive (called twice)
and in the case of exitValue to handle the IllegalStateException.
Some of the overhead could be avoided, by implementing toString in
each
of the ProcessImpl.java
files where the current state is known more conveniently with less
overhead.
$.02, Roger
On 5/8/2016 2:47 PM, Andrey Dyachkov wrote:
> Hello,
>
> I have added toString() method in Process.java.
>
> diff --git a/src/java.base/share/classes/java/lang/Process.java
> b/src/java.base/share/classes/java/lang/Process.java
> --- a/src/java.base/share/classes/java/lang/Process.java
> +++ b/src/java.base/share/classes/java/lang/Process.java
> @@ -548,5 +548,16 @@
> return toHandle().descendants();
> }
>
> + @Override
> + public String toString() {
> + boolean isAlive = this.isAlive();
> + return new
> StringBuilder(this.getClass().getSimpleName()).append("[")
> + .append("running=").append(isAlive).append(", ")
> + .append(isAlive ? "pid=" : "exitCode=")
> + .append(isAlive ? this.getPid() :
this.exitValue())
> + .append("]")
> + .toString();
> + }
> +
>
> }
--
With great enthusiasm,
Andrey
