JDK 8 Warnings Cleanup Day -- Dec. 1st, 2011

[Stuart Marks]

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<URL> 
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<String> 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