Roger,
I have changed impl according to your comments, thank you!
Regarding fc-extension, I am not able to add comments in the bug
tracker I think you can do it, correct? Also I am already in OCA list.
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,19 @@
return !hasExited;
}
+ /**
+ * The {@code toString} method returns a string consisting of
+ * the native process ID of the process and the exit value of the
process.
+ *
+ * @return a string representation of the object.
+ */
+ @Override
+ public String toString() {
+ return new StringBuilder("Process[pid=").append(pid)
+ .append(", exitValue=").append(hasExited ? exitcode : "\"not
exited\"")
+ .append("]").toString();
+ }
+
private static native void init();
static {
diff --git a/src/java.base/windows/classes/java/lang/ProcessImpl.java
b/src/java.base/windows/classes/java/lang/ProcessImpl.java
--- a/src/java.base/windows/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/windows/classes/java/lang/ProcessImpl.java
@@ -564,6 +564,20 @@
private static native boolean isProcessAlive(long handle);
/**
+ * The {@code toString} method returns a string consisting of
+ * the native process ID of the process and the exit value of the
process.
+ *
+ * @return a string representation of the object.
+ */
+ @Override
+ public String toString() {
+ int exitCode = getExitCodeProcess(handle);
+ return new StringBuilder("Process[pid=").append(getPid())
+ .append(", exitValue=").append(exitCode ==
STILL_ACTIVE ? "\"not exited\"" : exitCode)
+ .append("]").toString();
+ }
+
+ /**
* Create a process using the win32 function CreateProcess.
* The method is synchronized due to MS kb315939 problem.
* All native handles should restore the inherit flag at the end
of call.
On Mon, 1 Aug 2016 at 19:34 Roger Riggs <roger.ri...@oracle.com
<mailto:roger.ri...@oracle.com>> wrote:
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
--
With great enthusiasm,
Andrey
