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

[Sean Mullan]

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: http://cr.openjdk.java.net/~sherman/kurchi/7088502/webrev/
> Bug description: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7088502

My comments follow, line numbers are in [].

make/com/sun/crypto/provider/Makefile
make/java/security/Makefile
make/javax/others/Makefile
src/share/classes/com/sun/org/apache/xml/internal/security/keys/keyresolver/implementations/X509SKIResolver.java

There are no diffs for these files.

* HelperNodeList.java

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

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

* X509IssuerSerialResolver.java

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

* TransformXSLT.java

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

tFactory.setFeature("http://javax.xml.XMLConstants/feature/secure-processing", 
Boolean.TRUE);

* MessageDigestAlgorithm.java

[74-5] This is preferable:

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

* ElementProxy.java

[497-8] This is preferable:

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

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

* InclusiveNamespaces.java

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

         for (String prefix : prefixList) {

* Canonicalizer.java

[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();

* Transform.java

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

[234] Change to

Class<? extends TransformSpi> registeredClass = 
getImplementingClass(algorithmURI);

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

* IdResolver.java

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

* Manifest.java

[72] Change to Map

* TransformXPath2Filter.java

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

* XMLUtils.java

[325-7} change to for/each loop

* Canonicalizer20010315.java

[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:

@SuppressWarnings("unchecked")
...

  ns.getUnrenderedNodes(result);


* SignatureAlgorithm.java

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

[273, 332] Change type to Map

* KeyInfo.java

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

* KeyResolver.java

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

* Canonicalizer11.java

[256] same comment as Canonicalizer20010315.java


--Sean