review request for 7008728: diamond conversion of basic security, permission, authentication code

[Stuart Marks]

On 12/27/10 12:15 PM, Sean Mullan wrote:
> On 12/22/10 8:39 PM, Stuart Marks wrote:
>> http://cr.openjdk.java.net/~smarks/reviews/7008728/webrev.0/
>
> src/share/classes/com/sun/security/auth/SubjectCodeSource.java
>
> [96] Looks like the converter also missed another one. I think this line
> should be:
>
> new LinkedList<>() :

Heh. Interesting. Probably has something to do with the occurrence within a
ternary operator (?:) or the presence/absence of a constructor argument. OK,
I'll collect a set of cases that the converter seems to have missed.

>> In particular, take a look at
>>
>> src/share/classes/javax/security/auth/Subject.java
>>
>> It looks like the converter might have missed a diamond opportunity at
>> line 157. Hmmm. Let me know what you think.
>
> Yes, I agree.
>
> Everything else looks good, though I did not scan every source file looking
> for potential missed conversions - I only looked for missed ones if it was
> near the code that had differences (such as SubjectCodeSource).

Yeah, it's pretty difficult to find diamond opportunities by eye, unless
similar cases are next to each other and some aren't converted.

> Also, I would be curious to know if there has been any discussion in Project
>  Coin regarding my other comment as to whether the diamond operator improves
>  code readability if the variable is declared elsewhere.

I'm not aware of any such discussions. (I haven't been following Coin that
closely until recently, however,

Of course, there has been a lot of discussion about the type inference
algorithm, and the applicability of diamond at certain locations in the code.
In particular the use of diamond in a method argument seems to be problematic.
(I don't fully understand why. I think it has something to do with the
complexity of the inference algorithm vs. method overload resolution, and
possibly also with type inference that occurs if the method itself is generic.) 
We may end up avoiding diamond conversion in this case.

I'm also sure there has been discussion of the use of diamond in toy programs.
Certainly for toy programs (e.g., those that fit on a single slide) it makes
the code shorter and less redundant, and in isolated cases (such as those that
fit on a single slide) this is most probably better. But in real code, it's not
so clear.

One of the points of the diamond conversion project (and indeed the rest of the
Coinification effort as well) is to try out the constructs on real code and see
how well they work or don't work, to see where the constructs should and should
not be applied, and if possible to develop a style of recommended usage.

In the thread for the review of 7008713, you had asked:

> One instance where I think it is debatable as to whether it improves code
> readability is if the variable is declared somewhere else in the code. In
> these cases I find myself scrolling upwards or searching to find the
> declaration to see what type the parameters are.

Yes, this is certainly debatable and worthy of discussion.

Let's look at a couple specific examples. In the webrev for this change, the 
first file (PolicyFile.java) has a couple good examples. At line line 1183 
there's an example of a diamond used in the initializer for a local variable:

     ArrayList<Certificate> userCertList = new ArrayList<>();

It's pretty clear the new ArrayList contains instances of Certificate, since 
the declaration is right there. However, earlier in the same file at line 295, 
there's the following change (shown in unified diff format):

-        policyEntries = new Vector<PolicyEntry>();
+        policyEntries = new Vector<>();

In this case policyEntries is a field of the enclosing object. Looking at this 
diff, it's not at all clear that this change is correct. In order to determine 
its correctness, you have to hunt around in the file to find the declaration.

But note that policyEntries isn't actually used here so the type parameter 
isn't really significant for this particular bit of code. The type parameter 
*is* significant at other places where policyEntries is actually used (e.g., 
lines 651 and 911), and these are also pretty far from the field's declaration 
-- and these uses are also independent of the diamond operator. So, in order to 
use the field properly, you have to hunt around for the declaration (or ask 
your IDE to find it for you) anyway.

There are probably cases where a field might be declared somewhere, but then 
there's some code that constructs a new instance using the diamond operator and 
stores it in that field, and then uses it immediately. In that case it might be 
helpful to have the type parameters in the constructor since the code right 
below it uses them. Are there any such cases in the code? Probably. I could go 
through more of the code to look for cases like this.

What do you think? Do we want to avoid using diamond in straight assignment 
statements (where the declaration is far away) or is it OK? Would this be a 
general rule, or would it be applied on a case-by-case basis, depending on some 
criterion, e.g. how far away the declaration is?

s'marks