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