Hi Chris,
On 5/06/2015 9:34 AM, Chris Plummer wrote:
On 6/3/15 11:31 PM, David Holmes wrote:
Typo ...
On 4/06/2015 4:04 PM, David Holmes wrote:
Hi Chris,
On 3/06/2015 1:20 PM, Chris Plummer wrote:
Please review the following:
Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8081771
This review only concerns the changes to ProcessTool.java. The
Your new method needs javadoc explaining its usage, and in particular
what addTestVmAndJavaOptions actually refers to. Also a comment on why
the explicit -cp is added, but on under that arg, would be useful.
but only under that arg ...
Hi David,
Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8054386/webrev.04/
Note, this is the first time I've ever done javadocs, and I'm not even
sure how to build the javadocs to make sure it formats correctly. So if
anything looks suspicious and you think needs verification, please let
me know how.
I've never built javadocs for the tests/testlibrary either, but simply
read them as code comments. What you have done is consistent with the
other doc comments in the file (which have some style inconsistencies
with other JDK code, but not enough to worry about here).
The comment on -cp is somewhat longer than I had anticipated - might be
one of the rare occasions where a reference to the CR would be better.
But unless someone else complains ...
Thanks,
David
thanks,
Chris
David
Thanks,
David
CDSJDITests and filemapp.cpp changes will be committed under CR
JDK-8054386, but they rely on this change to ProcessTool.java, so both
CRs will be pushed together. See the following thread for the
JDK-8054386 review:
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014923.html
thanks,
Chris
