On 9/5/2013 11:51 AM, Daniel Fuchs wrote:
<http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.02/>
Logger.java - I like this and much cleaner fix.
251 // because global.manager will become not null somewhen during
252 // the initialization of LogManager.
s/somewhen/somewhere/
The comment for the setLogManager method needs updated since it's
called when a logger is registered to a log manager.
347 // It is called from the LogManager.<clinit> to complete
348 // initialization of the global Logger.
The checkPermission() method has this code to deal with null manager:
if (manager == null) {
// Complete initialization of the global Logger.
manager = LogManager.getLogManager();
}
I think it's still possible that checkPermission is called with
null manager, right? e.g. calling Logger.global.setFilter
Should this simply call LogManager.getLogManager() unconditionally as
in getGlobal()?
LogManager.java - the initialization is really tricky and the discussion
on the ensureLogManagerInitialized is very useful.
159 private volatile Logger rootLogger; // non final field - make it volatile to
160 // make sure that other threads will see the new value once
161 // ensureLogManagerInitialized() has finished executing.
suggest to move the comments above the declaration
Maybe renaming "initializedCalled" to "recursiveInit" to make the 2
variables easy to tell. space missing around "=" in line 270, 271
310 // Read configuration. This was previously
triggered
311 // by the new RootLogger() constructor - but no
longer.
Previously readPrimordialConfiguration() wascalled by LogManager.getLogManager
that was triggered when instantiating the RootLogger - to be precise.
It seems that leaving out the history might cause less confusion
since it no longer calls it.
760 final LogManager owner = getOwner();
761 logger.setLogManager(owner);
Should this have an assert to ensure logger.manager == null or == owner?
We don't expect a Logger to change its owner, do we? The behavior of multiple
LogManager instances is not specified anyway.
Reading the RootLogger - looks like it's potential for cleanup.
Logger.global uses a private constructor that doesn't call
LogManager.getLogManager to break the cyclic dependency. Perhaps
RootLogger should do the same? It's fine to leave it for future
if you prefer since you have well tested the bits you have.
Mandy
