review of 7117249: java.util warnings patches from LJC/Mike Barker
mikeb01 at gmail.com
Sat Dec 3 22:32:30 UTC 2011
Thanks for picking up the review.
> - java/util/jar/JarFile.java
> - java/util/jar/Manifest.java [patch4]
> - java/util/logging/LogManager.java [patch2]
> - java/util/prefs/Preferences.java
> - java/util/prefs/XmlSupport.java
> - java/util/zip/ZipEntry.java [patch2]
> Did I get everything? I think you've addressed all the review comments that
> have come in so far. (Other reviewers, please recheck the webrev!)
Yep, that looks like all of them.
> In addition, I have the following comments:
> JarFile.java --
> 242 ZipEntry ze = (ZipEntry)enum_.nextElement();
> 621 ZipEntry ze = (ZipEntry) enum_.nextElement();
> 676 ZipEntry e = (ZipEntry) entries.nextElement();
> I think these are all unnecessary casts now, since at 242 and 621 the return
> type from nextElement is <? extends ZipEntry> and at 676 it's JarEntry, and
> JarEntry is a subclass of ZipEntry. So, these casts can probably be removed
> even though they don't generate warnings. An alternative would be to change
> the type of e at 676 to JarEntry; not sure if this would be better.
> LogManager.java --
> 203 Logger.getGlobal().setLogManager(manager);
> 204 manager.addLogger(Logger.getGlobal());
> The doc recommends replacing mentions of the global field with a call to
> getGlobal(), and this was done here, however sometimes what's in the doc
> doesn't necessarily apply to library code. Can someone who's more familiar
> with logging verify this change is correct?
> 418 Class<?> clz = ClassLoader.getSystemClassLoader().loadClass(word);
> 419 Handler hdl = (Handler) clz.newInstance();
> Minor nit: extra space was before 'clz' in the original code, I think, to
> make it line up with 'hdl'. Maybe spacing needs to be rearranged to keep
> them lined up. (I see this a couple other places in the file, but it's not
> too prevalent.) The alternative is to have a single space between the type
> and the variable name.
These all look sensible, I'll implement them.
> Mike, if you end up needing to update this patch further, it might be
> easiest if you just sent all the changes in a single patch file, i.e. the
> output of 'hg diff'. I can then apply it to the tip and generate a webrev
> quite easily.
No problem. I'll drop a new patch in tomorrow.
More information about the core-libs-dev