8178380: Module system implementation refresh (5/2017 update)

[Alan Bateman]

On 02/05/2017 03:47, Mandy Chung wrote:

> :
> src/share/vm/services/attachListener.cpp
>
>   355 // Dynamic loading agents is not default by default
>
> typo: s/not default/not enabled?
I agree, the comment is confused and better to remove it.

>
> src/share/vm/services/diagnosticCommand.cpp
>   771     loadAgentModule(CHECK);
>   772     Handle loader = Handle(THREAD, SystemDictionary::java_system_loader());
>   773     Klass* k = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_agent_Agent(), loader, Handle(), true, CHECK);
>   774     instanceKlassHandle ik (THREAD, k);
>
> It’s better to refactor this to loadAgentClass that returns Klass or handle?
I agree although the duplicate code seems to have been there for some 
time, I think better if we create an issue to have it cleaned up in JDK 10.

>
> src/java.base/share/classes/java/lang/ClassLoader.java
>
>   134  *     platform class loader may delegate to application class loader.
>
> “the” application class loader
>
>   135  *     In other words, classes in modules defined to the application class
>   136  *     loader may be visible to the platform class loader. </li>
>
> It would help if this statement makes it clear that the classes in modules defined to the application class loader (only that are required by the upgradeable modules) are visible to the platform class loader.
There is a typo in the javadoc, it should be "in named modules" and I 
think that will make it much clearer.


>
> It would also be good to mention that ClassLoader::getPackages will not
> return the packages defined to the application class loader since
> application class loader is not its ancestor.
>
>   377      * @apiNote If the parent is specified as {@code null} (for the
>   378      * bootstrap class loader) then there is no guarantee that all platform
>   379      * classes are visible.
>
> What about:
>
> If the parent is specified as {@code null} (for the bootstrap class loader)
> then there is no guarantee that all platform classes are visible.
> Platform classes defined by the platform class loader and its ancestors
> except bootstrap class loader are not visible to this class loader.
I think this would clutter the class description and it might be better 
if we added a note to the getPackages method.


>
> src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java
>
>   174     /**
>   175      * Register a module this this class loader. This has the effect of making
>   176      * the types in the module visible.
>   177      */
>   178     public void loadModule(ModuleReference mref) {
>
> Typo in line 175 “this this class loader”.
Got it - thanks.

>
> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java
>
> 147         // special mode to boot with only java.base, ignores other options
>   148         if (System.getProperty("jdk.module.minimumBoot") != null) {
>   149             return createMinimalBootLayer();
>   150         }
Is there an issue here?


>
> src/java.base/share/classes/jdk/internal/module/ModulePatcher.java
>   123                     try (JarFile jf = new JarFile(file.toString())) {
> - what is the bug being fixed by this line?
This is part of the issue that Remi brought up last week related to 
overriding the default file system provider. I've replaced 
test/java/nio/file/spi/SetDefaultProvider.java with a more comprehensive 
test to test overriding with exploded and modular JAR that are patched / 
not-patched.


>
> src/java.base/share/classes/jdk/internal/module/ModulePath.java
>   536                     if (packages.contains(pn)) builder.mainClass(mainClass);
> Nit: it would be consistent if this is broken into two lines.
Okay.

>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>   960         bootLayer.modules().stream()
>   961             .map(m -> cf.findModule(m.getName()))
>   962             .flatMap(Optional::stream)
>
> This can be replaced with cf.modules().stream()
Okay.


>
>   988             .map(e -> Stream.concat(Stream.of(e.source()),
>   989                     toStringStream(e.modifiers()))
> Formatting nit: line 989 should be indented more to the right
I think the IDE moved it :-)


>
> 1064      * image ot be less than modules than not in the run-time image.
>
> typo: “ot”
Okay.
>
> Should these new tracing methods drop the printToStderr argument?
> This argument exists for compatibility to print help message to stderr.
> These new tracing options will print to stdout.
I was trying to keep it consistent but I think you are right, we should 
just init the output to System.out in these supporting methods.


> For validate modules,
> you suggested to print the errors to stderr which I agree.
I think but it's not in jake yet.

>
> src/java.base/share/classes/sun/launcher/resources/launcher.properties
>    60 \    --limit-modules <module name>[,<module name>...]\n\
>    61 \                  limit the universe of observable modules\n\
>
> -—limit-modules is intended for testing purpose.  I wonder if
> this should be moved to —-extra-help.
I've also been thinking it should move.


>
>    72 \                  The --validate-modules option may be is useful for finding\n\
>
> typo: “may be is useful”
Okay.


Thanks for going through all the changes. I will publish a new set of 
webrevs tomorrow once we have all the changes.

-Alan