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

[Weijun Wang]

Hi Stuart

In fact, I'm quite OK with the previous webrev. There can be more tweaks 
or manual editing, but not too necessary.

As for writing codes as a human, I will certainly use it in the 
initializer, where it looks ugly with the same parameter appears twice 
in a single line of code. As for other cases, I don't know yet. If 
NetBeans keeps on showing warnings for all possible coinifications, I 
might start use it everywhere, the yellow bulb is really frustrating.

And I prefer HashSet<Pair<String, String>>, my rule is that space is 
only inserted after "," unless *all* parameters have only one letter.

Thanks
Max


On 01/12/2011 11:04 AM, Stuart Marks wrote:
> 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