review request for 7005608: diamond conversion of JCA and crypto providers

Stuart Marks stuart.marks at oracle.com
Thu Dec 23 20:40:22 UTC 2010


On 12/22/10 6:17 PM, Brad Wetmore wrote:
> You need to update the Copyright updates on these files to include 2010.

Is this the standard, to update the copyright to the current year anytime the 
file is touched? If so, then yes, I can do this.

> Minor nit, can you add a space in line 221 between "BigInteger,BlindingParameters"

[and Max replied]
> I thought the formal style is no-space. At least in Map.java it's public interface Map<K,V>.

I'm not familiar with any formal style. The original Java code conventions doc 
[1] hasn't been updated since 1999 and so doesn't cover generics. I didn't see 
a more recent version.

The code base isn't consistent. I found about 3,000 uses of Map<...,...> in the 
JDK, and about 1,800 of those had a space after the comma and about 1,200 did 
not. To my eye it seems that single-letter type parameters such as Map<K,V> 
tend not to have the space whereas type parameters that name actual types, e.g. 
Map<String, Element> tend to have the space in there. However, the code base 
isn't consistent on that either.

I'm happy to change the code to <BigInteger, BlindingParameters> though.

[1] http://www.oracle.com/technetwork/java/codeconv-138413.html

> Not having a lot of experience yet with <>, the only ones I wasn't sure about
> were the ones in X509Factor.parseX509orPKCS7Cert. (line 415 & 425) I assume it
> just picks up the outer return type?

[and Max replied]
> I read the byte codes generated, and the parameters on lines 415 and 425 are not used. Maybe they are good for code readability.

Recall that generics are implemented by erasure, so no type parameters will end 
up in the bytecodes in any case. In fact, doing just the diamond conversion 
results in class files that are byte-for-byte identical to those from builds 
without the diamond conversion.

(The class files that result from building the code base with my webrev applied 
will in fact differ, though, because some of the lines have been joined where 
they're short enough. This will change the line number information in the class 
file, though it shouldn't change the actual bytecodes.)

But the issue is potentially significant in the source code. Here's the method 
in question, condensed for brevity. Lines changed by the diamond conversion are 
marked with **1** and so forth.

(Note also that X509CertImpl extends X509Certificate extends Certificate.)

     Collection<? extends Certificate> parse() {
         Collection<X509CertImpl> coll = new ArrayList<X509CertImpl>(); // **1**
         if (...)
             return new ArrayList<X509CertImpl>(0); // **2**
         try {
             X509Certificate[] certs = ...;
             if (...)
                 return Arrays.asList(certs);
             else
                 return new ArrayList<X509Certificate>(0); // **3**
         } catch {
             to coll, add instances of X509CertImpl
         }
         return coll;

At **1** if diamond were used, the type argument would clearly be X509CertImpl 
since that's right there in the declaration of the local variable.

At **2** and **3** (lines 415 and 425 in the file), when converted to use 
diamond, the type argument is inferred from the return type of the method. 
Since the return type is a bounded wildcard, the inferred type argument will be 
the upper bound, which is Certificate.

In these cases of diamond conversion, the inferred type will actually be 
different from what had been written before. Specifically, changing **2** to 
use diamond would be like changing it from

     return new ArrayList<X509CertImpl>(0);
to
     return new ArrayList<Certificate>(0);

and changing **3** to use diamond would be like changing it from

     return new ArrayList<X509Certificate>(0);
to
     return new ArrayList<Certificate>(0);

Now, what's the significance of this change? Would it be incorrect or 
misleading to use the diamond operator in the cases?

It's probably not incorrect from a type-safety point of view, since if it were, 
the compiler would issue an error or warning.

I guess the question is whether it's significant to the code in question 
whether in one case an ArrayList<X509CertImpl> is returned and in another case
ArrayList<X509Certificate> is returned, and also whether it's significant if 
these were both changed to ArrayList<Certificate>. To my eye, since the 
ArrayList is created empty and not otherwise used before it's returned, it's 
not significant. However, you guys (the maintainers of the code) are really the 
ones who need to decide.

s'marks








More information about the security-dev mailing list