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

[Eric McCorkle]

These have been addressed.  A new webrev has been posted.

On 08/21/13 17:52, Alex Buckley wrote:
> 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
>>>>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 303 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130822/bb31cf70/eric_mccorkle.vcf