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.
