Hi Chris,
Thanks for the review and comments.
On 3/24/2015 10:42 AM, Chris Hegarty wrote:
Roger,
I think the API is looking much better now ( just a few comments below on small
specific issues ), so I’ve taken a pass over the implementation changes in the
sandbox. Here are some comments:
1) Some operations on ProcessHandle require RuntimePermission
"manageProcess", but I don't see this specified in
java.lang.RuntimePermission ( there is a table of target runtime
permission names ). I think you just need to add it.
Yes will add
2) Can ProcessImpl.waitForProcessExit and its native counterpart be
removed? ( since its function is now performed through ProcessHandleImpl )
I'll look at that again; the two behaviors:
1) waiting for and reaping the exit value used by Process passes a
pid/windows handle is one case,
2) waiting for a pid and not touching the exit value passing a pid is
the other used by ProcessHandle.
It currently passes a flag argument but may be better as two different
native methods,
considering that Process no longer extends ProcessHandle.
3) "process reaper”, maybe
“Process-Reaper-“ + monotonically increasing ID ( similar to
ForkJoin worker threads, or other ). But I do accept that this
is the existing name for the reaper threads.
Makes sense; most thread names use mixed case;
I notice many thread names use spaces in the names instead of '-'s.
4) Should Info.toString() print something if all fields are null,
or -1.
ok, but localization of any words would be an issue.
I'd propose to put '[]' brackets around the whole string so it will
empty '[]' if none.
5) Could the fields for Info be private final, and use a separate
private holder for retrieving the information from native?
Seems desirable for them to be final.
The implementation class is package private and only exposes the Info
interface.
What is the concern? adding another class doesn't seem worth the overhead
or complexity.
6) Process has s number of @implXXX tags, they typically appear
before the @return in usage I’ve seen in the JDK.
ok
7) Should ProcessHandleImpl.toString include Info, and/or other
information about the handle?
The toString is just the PID, retrieving the rest of the info is
relatively slow,
involving a native call and creating objects. I think simple is best.
Subjective/Minor:
1) There are number of commented out fprintfs in native code
that should be removed.
Yes, still cleaning the sandbox
2) You may need to take a pass over the copyright year range.
I understand that some of these files were created in 2014, but
I suspect that they should all at least include 2015.
ok
3) OnExitTest & InfoTest include the “Classpath” exception, when
they should not.
ok
4) There is some duplication of javadoc in implementation
that could be removed, e.g:
/**
* Returns the process ID as a decimal String.
* @return the process ID as a decimal String
*/
My preference is to not repeat the public API doc in
implementation classes, but I accept that this is sometimes
done for in-place readability.
ok, will cleanup the comments
5) There are number of lines that are greater than 80
characters. These can be difficult for future maintenance
and reviews ( webrev side-by-side diffs ).
Good point, I'll wrap them.
I need to spend a little more time on the native code, but the tests pass so
that is a good start!
Thanks, Roger
