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

[Michael Barker]

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.