RFR: 8148568: LoggerFinder.getLogger and LoggerFinder.getLocalizedLogger should take a Module argument instead of a Class.

[Mandy Chung]

> On Apr 13, 2016, at 10:28 AM, Daniel Fuchs <daniel.fuchs at 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.fuchs at 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