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

[Stuart Marks]

Great, thanks, I'll go ahead and push the change.

Regarding NetBeans and warnings, I've been in contact with the NetBeans folks 
and I've asked them to make the hint engine configurable. It's quite clear from 
this and the foregoing discussion that we want to use diamond in certain cases 
but not others. Having NB raise warnings everywhere diamond can possibly be 
used is clearly inappropriate. I don't know exactly how it will end up, but 
maybe the warnings will be configurable, or maybe there will be a batch mode 
"diamond finder" that will offer to apply diamond to a list of sites, and you 
can pick and choose where you want it applied.

s'marks


On 1/11/11 7:30 PM, Weijun Wang wrote:
> 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