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

thanks,

Chris