8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

On 08/02/2017 03:10, Paul Sandoz wrote:

> Hi,
>
> Just minor stuff in the JDK area (i browsed quickly through the tests).
Thanks, a few comments below.

>
> Paul.
>
>
> java.lang.module.Configuration
> —
>
>   321      * @implNote In the implementation then observability of modules may depend
>
> s/then/the
"then" should be okay, it could also be "then the" (that might be what 
you mean).




> :
>
> Potential optimisation. Convert the sets to arrays, sort ‘em, then use Arrays.compare.
Yes, that could work. It's a real corner case to have to do this but we 
can do better for such cases.

>
>
> BuiltinClassLoader
> —
>
>   925     private ModuleReader moduleReaderFor(ModuleReference mref) {
>   926         return moduleToReader.computeIfAbsent(mref, m -> createModuleReader(m));
>   927     }
>
> Use this:: createModuleReader
I'll defer to Claes on this, mostly because #classes triggered to load 
here is observable in startup tests.

>
>
> jdk.internal.module.Builder
> —
>
>   279         Set<ModuleDescriptor.Modifier> modifiers;
>   280         if (open || synthetic) {
>   281             modifiers = new HashSet<>();
>   282             if (open) modifiers.add(ModuleDescriptor.Modifier.OPEN);
>   283             if (synthetic) modifiers.add(ModuleDescriptor.Modifier.SYNTHETIC);
>   284             modifiers = Collections.unmodifiableSet(modifiers);
>   285         } else {
>   286             modifiers = Collections.emptySet();
>   287         }
>
> It would be nice to use the small collection methods given the sizes. The following might work:
This is jlink plugin and would be really odd to have modules with the 
synthetic modifier. There are no open modules in the JDK images but a 
custom image may include some so point taken, this could be more efficient.


> :
>
>
> test/tools/jar/modularJar/Basic.java
> —
>
>   331     @Test(enabled = false)
>   332     public void partialUpdateFooMainClass() throws IOException {
>
> Just checking if that test still needs to be disabled.
There are fixes going into jdk9/dev for the jar tool that should allow 
us to re-enable this, it all depends on when the changes will meet up. 
If we have to push with that test disabled then I'll make sure there is 
a bug.


> :
>
> —
>
> Separately, we missed the opportunity to add lexicographical comparison and mismatch to List (we added them for arrays).
>
Yes.

-Alan.