Code review request: 7088502 Security libraries don't build with javac -Werror

Sean Mullan sean.mullan at
Mon Sep 19 12:23:58 PDT 2011

On 09/16/2011 06:34 PM, Kurchi Hazra wrote:
> Hi Sean,
> Can you please review these changes?
> Summary: * Small changes to Java files in
> src/share/classes/com/sun/org/apache/xml/internal/security and its
> subpackages to remove build warnings.
> * Small changes to relevant makefiles to prevent reintroduction of
> removed warnings.
> webrev:
> Bug description:

My comments follow, line numbers are in [].


There are no diffs for these files.


[37] This declaration (using supertype) is preferable:

List<Node> nodes = new ArrayList<Node>();


I don't think any changes are necessary to this file.


[66, 122-3] Can you remove line 66, and replace 122-3 with:



[74-5] This is preferable:

static ThreadLocal<Map<String, MessageDigest>> instances=new
     ThreadLocal<HashMap<String, MessageDigest>>() {
     protected Map<String, MessageDigest> ...


[497-8] This is preferable:

static Map<String, String> _prefixMappings = new HashMap<String,String>();
static Map<String, String> _prefixMappingsBindings = new 

Also, avoid using the diamond operator above since the Apache impl. 
still supports JDK 1.5+ and will make future integrations more difficult.


[85-88] Replace this with a for each block:

         for (String prefix : prefixList) {


[87] Declare as Map instead of HashMap. Also, I think this should be:

static Map<String, Class<? extends CanonicalizerSpi>> _canonicalizerHash;

Similar changes should be made to the other Class<?> declarations. And 
line 155 can then be changed to:

this.canonicalizerSpi = implementingClass.newInstance();


[68-9] Can you declare the types as Maps (instead of HashMaps)?

[234] Change to

Class<? extends TransformSpi> registeredClass = 

Also change Class types of line 334 and 345 to be consistent.


[55-6] Change to:

private static Map<Document, Map<String, WeakReference<Element>>> docMap =
     new WeakHashMap<Document, Map<String, WeakReference<Element>>>();

[74, 160] Change WeakHashMap to Map.


[72] Change to Map


[175-6, 289-90] change to for/each loop


[325-7} change to for/each loop


[209,314] I'm curious about these changes. Instead of adding a new 
method getSortedSetAsCollection and then suppressing the warnings for 
that, it seems like it would be sufficient to just suppress the warnings 
in this code, ex:




[130, 399, 434] change type to Class<? extends SignatureAlgorithmSpi>

[273, 332] Change type to Map


[103-5] Try changing this to:

     private static final List<StorageResolver> nullList;
     static {
         List<StorageResolver> list = new ArrayList<StorageResolver>(1);

Then I think you can remove the @SuppressWarnings("unchecked")
on line 1051.


[116-8, 159-62] change to for/each loop


[256] same comment as


More information about the security-dev mailing list