review of 7117249: java.util warnings patches from LJC/Mike Barker (original) (raw)

Michael Barker mikeb01 at gmail.com
Sat Dec 3 22:32:30 UTC 2011

Hi Stuart,

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.

Mike.

More information about the core-libs-dev mailing list