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> 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> 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
