please review fix for 7118546, warnings cleanup in javax.xml.crypto

[Sean Mullan]

On 12/7/11 8:38 PM, Stuart Marks wrote:
> 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.

Now that they are warning free, should there also be a change to the Makefile?

In javax/script/ScriptException.java, why is the serialVersionUID field not
marked private?

> 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.)

It looks good to me. I did not see any API changes.

> 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.

It looks good to me.

> 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.

All the other changes look good. The only other suggestion I would make is to
add a note to the CR with more rationale as to why the SuppressWarnings, etc
were added to the API so that future maintainers will understand that. In short,
this API was specified as a standalone JSR (105) and any API changes would need
to be first published via a maintenance review per the JCP guidelines.

Thanks,
Sean