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.
2) Can ProcessImpl.waitForProcessExit and its native counterpart be
removed? ( since its function is now performed through ProcessHandleImpl )
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.
4) Should Info.toString() print something if all fields are null,
or -1.
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.
6) Process has s number of @implXXX tags, they typically appear
before the @return in usage I’ve seen in the JDK.
7) Should ProcessHandleImpl.toString include Info, and/or other
information about the handle?
Subjective/Minor:
1) There are number of commented out fprintfs in native code
that should be removed.
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.
3) OnExitTest & InfoTest include the “Classpath” exception, when
they should not.
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.
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 ).
I need to spend a little more time on the native code, but the tests pass so
that is a good start!
-Chris.
