Webrev is here:
http://cr.openjdk.java.net/~smarks/reviews/7118546/webrev.0/
Notes:
In addition to changes to javax.xml.crypto, this webrev also includes a change
to a file in javax.script. This package (among others) is included in the same
build step as javax.xml.crypto, specifically in jdk/make/javax/others/Makefile.
With this change, this entire build step will become warning-free.
Many of the changes are simply adding @SuppressWarnings("rawtypes") at
locations where raw types are used in the API. I tried to be careful not to
change any actual APIs, but please recheck this. (During this warnings effort,
inadvertent API changes seem to be catching everyone.)
Of particular note are changes in the following files:
- ExcC14NParameterSpec.java
- XPathFilter2ParameterSpec.java
- XPathFilterParameterSpec.java
- XPathType.java
The constructors in these classes take a raw List or Map, make a defensive
copy, run through the elements and use instanceof to ensure they're all of the
right type, create an unmodifiable wrapper and then assign it to a field. The
current code uses all raw types.
(For the following discussion it probably would be useful to bring up the code
for the XPathType constructor.)
There were several alternative approaches to getting rid of the warnings in
this code. Since the code inevitably would have raw types and unchecked casts,
one approach would simply be to put @SuppressWarnings({"rawtypes","unchecked"})
at the top of the constructor and be done with it. However, this obscures up a
bunch of important details and IMHO covers too much code with @SuppressWarnings.
Another approach would be to do an unchecked cast (with warning suppressed)
directly from the raw type to the destination generic type such as
Map<String,String>. The code would then run through the entries to ensure that
each key and value is indeed a String. This seems odd, and might lead a future
maintenance programmer to "optimize away" such checks. Of course, these dynamic
type checks are important for the security of the system.
The approach I ended up taking was to make the defensive copy into a wildcarded
type such as Map<?,?> which I believe is an accurate type: it's a map, but we
don't know the actual types of its keys and values. Only after doing all the
instanceof checks do we do the cast to Map<String,String>. Unfortunately, the
resulting code is longer and has a proliferation of wildcards, making it appear
to be quite complex. But I think it actually makes the most sense of the
alternatives.
If we want to simplify this code, I'd suggest we convert these cases to use the
enhanced-for loop. I've refrained from doing so since we're avoiding
refactoring as part of the warnings cleanup exercise.
But let me know what you think.
s'marks
