Hi Mandy,
On 08/04/16 22:26, Mandy Chung wrote:
On Apr 8, 2016, at 7:52 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi,
Please find below a patch that slightly modifies
the JEP 264 initial implementation to take advantage
of the module system.
The change modifies the LoggerFinder abstract service
to take the Module of the caller class as parameter
instead of the caller class itself.
The caller class was used in the initial 9/dev implementation
because java.lang.reflect.Module was not yet available.
http://cr.openjdk.java.net/~dfuchs/jigsaw/webrev_8148568/webrev.01/
Using the Module as the caller granularity is reasonable here to find the
resource bundle.
DefaultLoggerFinder.java
135 static boolean isSystem(Module m) {
136 ClassLoader cl = AccessController.doPrivileged(new
PrivilegedAction<ClassLoader>() {
This isSystem method should check if the class loader is platform class loader
(ClassLoader::getPlatformClassLoader) or its ancestor.
If you don't mind I'd rather do this change in a followup fix.
There are many places (possibly including tests) where we use the
idiom getClassLoader() == null. All these place would need to be
audited and possibly changed, or not...
For consistency reason I'd prefer to start with isSystem
returning getClassLoader() == null. Then we can examine
whether isSystem should be relaxed to also accept
getClassLoader() == ClassLoader.getPlatformClassLoader(), and
which other places should do the same.
I'd feel safer if the changes getClassLoader() == null =>
getClassLoader() == null || getClassLoader() ==
ClassLoader.getPlatformClassLoader()
was made in a fix that only contains that.
There are several copies of this method. Better to have one single copy of this
method shared for other classes to use?
OK. I put it in DefaultLoggerFinder. That seems like a good place and
it could be imported from there.
Nit: you can use the diamond operation PrivilegedAction<>.
done.
LazyLoggers.java.
line 294 return new LazyLoggerAccessor(name, factories, module);
I spot several long lines in LogManager.java, Logger.java, System.java and
tests, maybe other files. It’d be good if you can wrap the lines to help
side-by-side review.
I fixed those. Tests might still have long lines.
http://cr.openjdk.java.net/~dfuchs/jigsaw/webrev_8148568/webrev.02/index.html
Other that that, looks good.
Mandy
