On 03/22/2013 10:05 AM, Laurent Bourgès wrote:
Hi Mandy & Peter,
Here is an update patch applying mandy's suggestions:
http://jmmc.fr/~bourgesl/share/webrev-2-8010309/
<http://jmmc.fr/%7Ebourgesl/share/webrev-2-8010309/>
You've beaten me! I have been preparing them too ;-) ...
https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/PlatformLogger/webrev.06/index.html
I also did some some renames, that I think make the code more consistent:
- LevelEnum -> Level (the code is not dependent on
java.util.logging.Level, so the name can be reused, its private anyway)
- julLevel -> javaLevel (javaLevel / JavaLogger)
- LevelEnum.forValue -> Level.valueOf (Mandy)
- JavaLogger.julLevelToEnum -> JavaLogger.javaLevelToLevel
Other changes (to webrev.05):
- removed the occurrence counts in switch comments (as per Mandy's
suggestion)
- made LoggerProxy and JavaLogger private
- fixed double-read of volatile LoggerProxy.levelValue in
LoggerProxy.isLoggable()
- added static Level.javaLevel(int value) shortcut (Mandy)
I also updated the test to exercise the correctness of mappings.
Have I forgotten something?
Regards, Peter
Good work and I like the solution.
Thanks.
JavaLogger and LoggerProxy are only used from within
PlatformLogger. Wouldn't it be better to declare them private?
Their usage (one or the other) depends on the 'loggingSupport'
flag that is initialized with PlatformLogger class. Usage of
JavaLogger or LoggerProxy outside PlatformLogger would be wrong,
I think.
Yes JavaLogger and LoggerProxy classes can be made private.
Done.
I looked at it but not in great detail. In general it's very good
clean up. The comment in line 124-132 is good information and
since the code is evolving, I would suggest to take those count out.
Fixed.
valueOf is one common method name to find an instance of a given
value. Perhaps LevelEnum.forValue can be renamed to
LevelEnum.valueOf(int)?
Done.
It seems to be useful to add a static method
LevelEnum.getLevel(int) to replace the calls to
"LevelEnum.forValue(newLevel).julLevel".
I agree I prefer using methods (field encapsulation) so I added:
+ /**
+ * java.util.logging.Level optionally initialized in JavaLogger's
static initializer
+ * and USED ONLY IN JavaLogger (only once java.util.logging is
available and enabled)
+ */
+ private Object julLevel;
+
+ void setJulLevel(Object julLevel) {
+ this.julLevel = julLevel;
+ }
+
+ Object getJulLevel() {
+ return this.julLevel;
+ }
+
+ /**
+ * Return the java.util.logging.Level used only in JavaLogger
+ * @param levelValue PlatformLogger level as int
+ * @return java.util.logging.Level or null if java.util.logging is not
available and disabled
+ */
+ static Object getJulLevel(int levelValue) {
+ return valueOf(levelValue).getJulLevel();
+ }
The tests are in jdk/test/java/util/logging and
jdk/test/sun/util/logging. It'd be good to check if you want to
extend jdk/test/sun/util/logging/PlatformLoggerTest.java to cover
all levels (it's currently already checking several).
I will now have a look ...
Regards,
Laurent
