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

Stuart Marks stuart.marks at oracle.com
Sat Dec 3 12:59:56 PST 2011

[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:

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

More information about the jdk8-dev mailing list