[9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

[David Holmes]

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
>