JDK 8 Warnings Cleanup Day -- Dec. 1st, 2011 (original) (raw)

Stuart Marks stuart.marks at oracle.com
Fri Dec 2 00:06:54 PST 2011

On 12/1/11 5:41 PM, Weijun Wang wrote:

(Incidentally, this is the third time I've reviewed code today that looks exactly like this. The other cases are in java.io.FilePermission and java.util.PropertyPermission. They each have the /FALLTHROUGH/ into a set of cases that do nothing but break; and they have similar ("ackbarfaccept") comments. It would be nice if these chunks of code could be unified, but they differ in a number of fiddly details.)

(The string "ackbarfaccept" occurs in the following files: 1. java/io/FilePermission.java 2. java/net/SocketPermission.java 3. java/util/PropertyPermission.java 4. javax/management/MBeanPermission.java 5. javax/security/auth/kerberos/ServicePermission.java Hmmm.) This is funny. We might do something later.

Yeah, this is a good thing to look at later. Looks like somebody long ago did some cut-and-paste programming. Brandon and Sebastian were working on FilePermission and PropertyPermission and might be considering how to merge/unify or at least coordinate the code. Now it seems that there are a few more instances to look at.

BTW, I've included some other sun.misc files and created a webrev at

http://cr.openjdk.java.net/~weijun/7116857/webrev.00/

Wow, lots more code. Comments below.

Some classes inside sun.misc are not touched, mainly SoftCache/Ref/Cache. I tried them but not very successful.

Sure, no problem doing just a subset of sun.misc.

Comments on webrev:

ClassLoaderUtil.java --

The generic types for loaders, urls, and lmap are kind of strange. (This code is kind of strange to begin with.) The urls variable is declared as Stack and you had to import java.net.URL for this; but the URL type isn't actually used anywhere. The lmap var is HashMap<String,?> but probably only because String is already implicitly imported, and it didn't make sense to import Loader (the type of the map's values). I guess since the values in any of these containers aren't actually used, maybe it makes sense to make them ALL wildcard types, i.e.

 ArrayList<?> loaders = ucp.loaders;
 Stack<?> urls = ucp.urls;
 HashMap<?,?> lmap = ucp.lmap;

Makes a bit more sense, in a curious way. (Assuming it works, of course.) Up to you whether you want to make it this way.

LRUCache.java --

You can reduce the scope of @SuppressWarnings by doing something like this:

 @SuppressWarnings("unchecked")
 V[] temp = (V[])new Object[size];
 oa = temp;

Queue.java --

Wow, another queue implementation. The generic conversion looks right. I'm just marveling at all the weird stuff off in the corner of sun.misc. Nothing to change here.

RequestProcessor.java --

Now that the Queue instance contained here has been generified, the code here can be assured that its contents are all subtypes of Request. There's logic in here that checks items for instanceof Request, ignores that aren't, and casts the remainder into Request. Just an observation that all this stuff is unnecessary. However, it's probably not worth changing this dusty old code at this point, if ever (and certainly not part of warnings fixes).

URLClassPath.java --

Now that jarFilesList is LinkedList instead of a raw LinkedList, it shouldn't be necessary to do any casts when fetching the contents. However, the existing code copies the contents to an Object[] and then downcasts the individual array elements to String. Ugh. This can be avoided by changing line 851ff to

 int size = jarFilesList.size();
 String[] jarFiles = jarFilesList.toArray(new String[size]);

**

If you have time today I'd appreciate it if you could build/test and push this change yourself. I'm backlogged reviewing and integrating changes from external folks.

thanks!

s'marks

More information about the jdk8-dev mailing list