review of 7117249: java.util warnings patches from LJC/Mike Barker

[Stuart Marks]

[bcc to jdk8-dev; subsequent reviews/comments should go to core-libs-dev]

Hi Mike, all,

I've collected all the patches together, filed bug 7117249 to cover them, and 
have started this new review thread on core-libs-dev for it. I've also posted a 
webrev for the collected patches:

http://cr.openjdk.java.net/~smarks/reviews/7117249/webrev.0/

It looks like we have the following patches from your group so far:

  - 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!)

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.

*******

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.

Thanks.

s'marks