review request for 7008713, update 1: diamond conversion of kerberos5 and security tools

[Stuart Marks]

Hi Max, thanks for reviewing the webrev.

On 1/11/11 5:25 PM, Weijun Wang wrote:
> On 01/12/2011 08:06 AM, Stuart Marks wrote:
>> I think there's one in a return statement in there too.
>
> You mean this one?
>
> public static <A,B> Pair<A,B> of(A a, B b) {
> - return new Pair<A,B>(a,b);
> + return new Pair<>(a,b);
> }
>
> I wonder if it should be included. The reason why people don't like "<>" in
> non-initializer, which I guess, is that it's too faraway from the declaration
> and people want to read the full generics parameters as a reminder. This is
> also true for a return statement when the method declaration is normally
> faraway from it.
>
> Of course, in this particular case, the return line is very near to the return
> type declaration, so it doesn't seem too bad. Just don't know how your
> automated conversion deals with other cases.

Yes, this is the one return statement I recall seeing. I think you've correctly 
identified the issue. For a return statement, the inferred type is the return 
type of the method, which is usually not too far away. In this case it's quite 
close but in other cases it could be pretty far away, but still at the top of 
the current method, which is usually not too hard to find. This isn't as bad as 
field assignment, whose declarations can be arbitrarily far away (though 
usually in the same file). It's even worse for method args, where the inferred 
type can even be in a different file.

The automated conversion tool applies diamond everywhere it possibly can. For 
this set of changes I've manually filtered out the changes we don't want to 
apply, such as assignment statements. I left this diamond on the return 
statement, though, since there didn't seem to be a compelling reason to take it 
out. Of course, diamond hardly buys anything here since it's just the type 
parameters <A,B>.

I can leave this in or take it out as you prefer.

(There is a newer version of the tool that allows one to configure which cases 
diamond is applied to. I'll start using that in subsequent rounds of diamond 
changes.)

>> I've adjusted whitespace that ended up around the diamond operator
>> itself. I think there's a pretty strong convention not to have any space
>> within or around a diamond. There are several places where I made
>> changes like the following:
>>
>> new HashMap<> () ===> new HashMap<>()
>
> Do you also change the declaration?
>
> Map <String> map ===> Map<String> map
>
> In src/share/classes/sun/security/tools/KeyTool.java, I see
>
> @@ -153,11 +153,11 @@
> ...
> - private List <String> ids = new ArrayList <String> (); // used in GENCRL
> - private List <String> v3ext = new ArrayList <String> ();
> + private List<String> ids = new ArrayList<>(); // used in GENCRL
> + private List<String> v3ext = new ArrayList<>();
>
> so, "List <String> v3ext" is changed to "List<String> v3ext".

Oh yes, good catch, I happened to see an extra space before the type arguments 
so I took it out. This was me making changes by hand, not the automated tool.

> but, in the same file,
>
> @@ -3873,8 +3873,7 @@
> break;
> case 2: // EKU
> if(value != null) {
> - Vector <ObjectIdentifier> v =
> - new Vector <ObjectIdentifier>();
> + Vector <ObjectIdentifier> v = new Vector<>();
> for (String s: value.split(",")) {
> int p = oneOf(s,
> "anyExtendedKeyUsage",
>
> Here, there's still a whitespace in "Vector <ObjectIdentifier>".

Yeah, I missed that one. I'd say the space after "Vector" should be removed.

> There are also lines in this file with only the declaration:
>
> 127: private Set<Pair <String, String>> providers = null;
> 499: providers = new HashSet<Pair <String, String>> (3);
> 663: for (Pair <String, String> provider: providers) {

These cases aren't as clear-cut. Sure, maybe there shouldn't be a space before 
the '<' but if there's a space after the comma (see below) then it makes things 
confusing.

>> For their webrevs both Sean and Brad had asked me to tweak whitespace
>> after the comma in the list of generic type arguments. For example, like
>> the following:
>>
>> Map<String,Object> ===> Map<String, Object>
>>
>> However, I did *not* make such changes in this webrev. I didn't know
>> whether you would have wanted such changes; there are rather a lot of
>> them to change, and it's also not clear to me whether it actually would
>> have improved the consistency of the spacing in this section of code.
>> But, let me know if you'd like me to adjust this whitespace as well.
>
> My style was no whitespace after "," but probably it's not good. Now I think
> <K,V> is OK but <String, String> is better. Page 130 of "Effective Java 2nd
> version" seems to support this.

Yeah, no space after the comma for cases like this

     Map<K,V>

seem reasonable, but space after comma for cases like this

     Map<String, String>

also seems reasonable. But what about more complex generic types? This code 
unfortunately has a lot of them. Like the examples you cite above, which is 
better? This

     HashSet<Pair<String, String>>

or

     HashSet<Pair <String, String>>

or even

     HashSet<Pair<String,String>>

? Actually there are even more complex cases such as this one from 
src/share/classes/sun/security/krb5/Config.java:

     Hashtable<String,Hashtable<String,Vector<String>>> temp = ...

Where should the spaces go in that? I'm not sure....

> Since this code change is about coinification, I really don't care about
> whitespace styles here.

Yeah the whitespace is basically a distraction from coinification.

I'm inclined to do the following:

  - use diamond in the return statement discussed above
  - remove whitespace from "Vector <ObjectIdentifier>"
  - leave whitespace in complex generic types alone

but I'm happy to do otherwise on these if you prefer, or to make other changes 
you might suggest.

Thanks.

s'marks

>
> Thanks
> Max
>
>>
>> Here's a link to the updated webrev.
>>
>> http://cr.openjdk.java.net/~smarks/reviews/7008713/webrev.1/
>>
>> Thanks!
>>
>> s'marks