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
