RFR 8165634: Support multiple --add-module options on the command line

[harold seigel]

Hi Gerard,

Thanks for the review!  Please see comments inline.

Harold


On 9/8/2016 12:24 PM, Gerard Ziemski wrote:
> hi Harold,
>
> The changes look fine. I have a couple of questions though:
>
> #1 Would the "test/runtime/modules/ModuleOptionsTest.java” be more robust if we included a case with a non-error return value that takes more than one module? Ex: “java --add-modules=java.base --add-modules=jdk.internal.reflect=ALL-UNNAMED -version”
The VM just converts each --add-modules option into a 
jdk.module.add.mods.<n> property that it passes back to the JDK.  The 
fact that the JDK throws an exception for the 
"--add-modules=i_dont_exist --add-modules=java.base" case means that the 
VM is successfully handling both --add-modules and returning both of 
their values to the JDK.   So I don't think an additional test case is 
needed.

Also, the JDK AddModsTest.java test has a multiple --add-modules test 
case that returns successfully.
>
> #2 Looking at the changes for jdk it appears we now also allow duplicate "--add-exports” and "--add-reads"? Does it also mean we allow duplicate "--add-modules”, Ex: “java --add-modules=java.base --add-modules=java.base -version”?
Thanks for also reviewing the JDK changes.  The core-libs reviewers 
asked that the support for duplicate --add-exports and --add-reads be 
removed from this webrev.  (See next revision.)  Duplicate --add-modules 
are allowed.
>
>
> cheers
>
>
>> On Sep 8, 2016, at 8:23 AM, harold seigel <harold.seigel at oracle.com> wrote:
>>
>> Hi,
>>
>> Please review this fix for JDK-8165634.  The fix changes the --add-modules option from being a 'last one wins' option to a cumulative one.  With this change, if multiple --add-modules options are specified, the VM accumulates all the options' values, instead of ignoring all but the last option's value.  The --add-modules values are reported back to the JDK as properties using the Arguments::create_numbered_property() function.
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8165634
>>
>> Open webrevs:
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/
>>
>> The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, java/util and other JTreg tests, the RBT tier2 tests, and the NSK non-colocated quick tests.
>>
>> The JDK changes were done by Mandy Chung (mchung).
>>
>> Thanks, Harold
>>
>>