Review request for JDK-8020745: Suspicious MethodParameters attribute generated for local classes capturing local variables

[Alex Buckley]

The major test cases are now covered, which is good.

You should add a multi-line comment which enumerates them. The @summary 
is just not enough.

You could get rid of some "line noise" by using array initializers:

   return new String[] { "this$1",
                         "innerparam" };

This will minimize the number of variables with general names like 
"names" and "types". Such variables already exist in Tester#check and 
you _really_ want to avoid a hiding scenario.

Tester#message is pretty obscure. I mean, you can't _force_ the methods 
overriding it to capture the 'message' variable of ParameterNames. It 
would be more direct for class InnerTester et al so simply declare their 
own fields which reference ParameterNames.message. Then you could drop 
Tester#message. Please document that ParameterNames.message exists only 
to be captured, and that javac currently happens to choose val$message 
for the name of the synthetic ctor parameter but it may choose another 
name in the future.

I don't see what Tester.field is doing.

Alex

On 8/21/2013 2:00 PM, Eric McCorkle wrote:
> I have addressed these issues and uploaded a new webrev.  Crucible
> reviews are also complete.
>
> On 08/20/13 21:17, Alex Buckley wrote:
>> - Method makeAnonExtendsInner does not return an instance of an
>> anonymous class whose superclass is inner. InnerList is a local class,
>> and its superclass ArrayList is top level. Also, you should document the
>> kinds of classes for which you're exercising parameter generation.
>>
>> - Encapsulate! The "expected" variables like Inner_names should be
>> closer to the classes declaring the "actual" variables (i.e. constructor
>> parameters) of interest. checkConstructor should merely have to ask that
>> class to check itself.
>>
>> Alex
>>
>> On 8/20/2013 3:36 PM, Eric McCorkle wrote:
>>> A new webrev has been uploaded which addresses these issues.
>>> http://cr.openjdk.java.net/~emc/8020745/
>>>
>>> On 08/19/13 20:28, Alex Buckley wrote:
>>>> The AnonymousParameters.java test is very confusing. Two classes with
>>>> main methods in the same compilation unit?!
>>>>
>>>> Bug 8020745 indicates a mismatch between parameter names and types, but
>>>> this test only checks names. Rather than relying on the order of names
>>>> from the JVM to match the 'names' array, you should match up names and
>>>> types explicitly.
>>>>
>>>> List is a bad name for a helper class, and the @summary is not very
>>>> helpful.
>>>>
>>>> Most importantly, there are no anonymous classes here - List is a local
>>>> class. The test should have an anonymous class too - in fact, an
>>>> anonymous class whose superclass is not inner and an anonymous class
>>>> whose superclass is inner. (See the notes in 8misc.pdf 2.2 8.8.9 for why
>>>> that matters.)
>>>>
>>>> Alex
>>>>
>>>> On 8/19/2013 4:31 PM, Eric McCorkle wrote:
>>>>> Hello,
>>>>>
>>>>> Please review this patch, which causes captured locals to be added to
>>>>> the end of a MethodParameters attribute, like they should.
>>>>>
>>>>> The webrev is here:
>>>>> http://cr.openjdk.java.net/~emc/8020745/
>>>>>
>>>>> The bug report is here:
>>>>> http://bugs.sun.com/view_bug.do?bug_id=8020745
>>>>>
>>>>> This review is also on the new Crucible system.
>>>>>
>>>>> Thanks,
>>>>> Eric
>>>>>