> On Oct 19, 2015, at 11:18 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/webrev/index.html
Looks good in general. Some minor comments:
Good to see limited privileges being used that limits the permissions when
invoking logger finder which is good.
LoggerFinderLoader.java
110 (PrivilegedAction<ServiceLoader<DefaultLoggerFinder>>)
- can this use diamond operation (PrivilegedAction<>)?
System.java
1547 return LazyLoggers.getLogger(Objects.requireNonNull(name), caller);
Objects.requireNonNull(name) - already called. It can just do getLogger(name,
caller).
RuntimePermission.java
360 * to JDK classes.</td>
s/JDK/system/
LazyLoggers.java
sun/management/ManagementFactoryHelper.java
172 public interface LoggingMXBean
173 extends PlatformLoggingMXBean, java.util.logging.LoggingMXBean {
174 }
LazyLoggers.java
68 this(Objects.requireNonNull(loggerSupplier),
69 (Void)null);
130 this(Objects.requireNonNull(name),
Objects.requireNonNull(factories),
131 Objects.requireNonNull(caller), null);
I would expect Objects.requireNonNull on the constructor initializes all the
fields. But the above lines checks non-null before calling this(….). Does the
other constructor allow null parameters? Why not moving Objects.requireNonNull
to the other constructor?
365 new BiFunction<String, Class<?>, Logger>() {
- it can use diamond operator
BootstrapLogger.DetectBackend looks like have some duplication as
LoggerFinderLoader.
- does it make sense to merge them?
923 boolean cleared;
- should that be volatile field?
AbstractLoggerWrapper.java
212 if (sourceClass==null && sourceMethod==null) {
234 if (sourceClass==null && sourceMethod==null) {
Missing space around “==“ and also in a few other lines.
java.util.logging.LogManager.demandLoggerFor
- I think this method is intended for the LoggerFinder provider to use.
Perhaps we should add a note in the javadoc to say that.
SimpleConsoleLogger.skipLoggingFrame and another place is finding the caller
information. JEP 259 may be useful for stack walking with filtering.
I skimmed through the tests and nothing pops up. There are multiple copies of
AccessSystemLogger that can be shared as I comment last time.
Mandy
