On 01/05/2016 23:27, Peter Levart wrote:
Hi,
I have some mostly stylish changes (which you can use or not)
regarding usage of Optional and Stream(s) in package java.lang.module:
http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/
It's good to do cleanup although our main focus here is to get through
the transition to the new policy on root modules and the new -Xpatch as
both are disruptive. We will continue to work in jake for a while as
there are still many areas that need iteration and we need to go through
a few rounds of API clean-up too.
...with some optimizations/cleanups/fixes:
ModuleFinder.compose:
- the composed ModuleFinder.findAll() is more efficient this way as it
doesn't need to call back to find().
True although if you don't mind, I'd prefer to skip this for because
this method is due to be changed in a round of API updates (see
JDK-8152650). The implementation will be replaced then.
ModulePath:
- there's unneeded overhead to call distinct() for a Stream that is
later collected into Collectors.toSet() as the collector already
uniquifies the elements.
- lines 432...437 (old) - you don't want to treat entries in
SERVICES_PREFIX directory that end with ".class" as classFiles - so I
changes the:
.filter(e -> (e.endsWith(".class") ||
e.startsWith(SERVICES_PREFIX)))
to:
.filter(s -> (s.endsWith(".class") ^
s.startsWith(SERVICES_PREFIX)))
This looks good, I'll bring this in.
ModuleReader:
- the default method read(String name) should close the InputStream
after reading from it, shouldn't it?
Yes it should, just hasn't been noticed because each of the
implementations implement this method. I don't know if you mean to put
the try on the same line but I'll make that a bit more readable before
pushing the patch.
ModuleReferences:
- no need for 'closed' flag in SafeCloseModuleReader to be volatile as
it is always accessed under lock (either read lock when reading or
write lock when writing).
You are right. There has been a few iterations on this code and I thin
this was left over.
Resolver:
- Implemented caching of ResolvedModule(s) as they are entered into
resolved graph so that there are no duplicates.
- Simplified last stage of iterative algorithm to build resolved
graph. There's no need to construct a map of changes and apply it
afterwards to 'g1' as changes can be applied individually for each
module 'm1' in the graph 'g1' as they are collected.
I would prefer not to change makeGraph in this round. You are right that
it could be more efficient, Claes pointed this out recently too and had
caching too. So something for a future round where it might be
completely replaced.
-Alan
