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

[Daniel Fuchs]

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...

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
>