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
