Hi Mandy,
Thanks for the feedback!
On 9/6/13 4:28 AM, Mandy Chung wrote:
>
> 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/
OK
>
>
> 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.
Ah. Right.
>
> 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()?
I don't think this would be required.
Maybe we could make checkPermission() static in LogManager?
But we might still need to call LogManager.getLogManager() to avoid
regression
in code using Logger.global directly...
> 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
OK
>
> 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.
OK
>
> 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.
I am concerned it could introduce regressions in applications that
use multiple instances of LogManager or subclasses of Logger.
I agree this is not perfect. Unfortunately I don't see any ideal solution.
> 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.
It does. I mean it no longer uses the two args constructor that calls
LogManager.getLogManager().
>
> Mandy
>
>