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

[Weijun Wang]

Hi Stuart

On 01/12/2011 08:06 AM, Stuart Marks wrote:
> Hi Max,
>
> Here, finally, is an updated webrev for 7008713. Like the other updates,
> this is the automated diamond conversion but with diamonds removed from
> assignment statements. This is mostly diamonds used in variable
> initializers. 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.

>
> 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".

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>".

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) {

>
> 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.

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

Thanks
Max

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