> On Apr 13, 2016, at 10:28 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > > 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…
That’s fine with me. > > 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. > In the practice this check is right. But the spec allows another non-null ancestor, if present, of platform class loader to define java.* classes. >> 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 > +1 Mandy
