Review request for JDK-8014230: Compilation incorrectly succeeds with inner class constructor with 254 parameters

[Alex Buckley]

Thanks ... just a few things:

- The fact that NumArgsTest enforces "threshold passes, threshold + 1 
fails" is buried quite deep in runTest. Perhaps a comment at threshold's 
declaration that it's the maximum number of parameters allowed?

- Maybe declare "static NestingDef[] NO_NESTING = {};" in NumArgsTest, 
then in ToplevelClass* and StaticMethodArgs just pass NO_NESTING rather 
than bothering with the nesting field.

Alex

On 6/20/2013 11:38 AM, Eric McCorkle wrote:
> Thank you for the suggestions, Alex.  I have applied all except the
> change to "threshold".  I named it as such because the test asserts that
> methods with "threshold" arguments should pass, but ones with
> "threshold" + 1 should fail.  It seems to exactly describe the meaning.
>
> A new webrev has been posted:
> http://cr.openjdk.java.net/~emc/8014230/
>
> Also, there are several successful JPRT runs (minus a known issue).
>
> Since Jon made comments earlier, I would like to hear from him before I
> commit.
>
> On 06/19/13 17:57, Alex Buckley wrote:
>> - There is less coverage because NumArgs1/2 tested instance and static
>> methods while the new MethodArgs only tests instance methods.
>>
>> - There is no such thing as a static inner class. An inner class is
>> non-static by definition (JLS7 8.1.3). The term StaticInnerClass should
>> be replaced with StaticNestedClass throughout.
>>
>> - For grouping purposes, it would be good to prefix ConstructorArgs and
>> MethodArgs with "ToplevelClass". Later, we'll have an "AnonClass" prefix
>> too.
>>
>> - Might rename threshold to maxArgs in NumArgs. (Is a threshold
>> something you can step up to but not on, or something you can step on?)
>>
>> - Might rename retty ("return type") to result, since the field can
>> store "void" and void is not a [return] type (JLS7 8.4.5)
>>
>> - Might rename ClassNestingDef to NestedClassBuilder.
>>
>> Alex
>>
>> On 6/19/2013 12:08 PM, Eric McCorkle wrote:
>>> Added a new constructor to Main that allows tests to pass in their own
>>> Log, facilitating much more precise tests of what errors the compilers
>>> generates.
>>>
>>> I also rolled all the arg limit tests into a general framework.
>>>
>>> Webrev is here:
>>> http://cr.openjdk.java.net/~emc/8014230/
>>>
>>> On 06/14/13 16:30, Jonathan Gibbons wrote:
>>>> There are two solutions.
>>>>
>>>> For a one-off test, the standard technique is to use a golden file
>>>> in conjunctions with rawDiagnostics.   The combination is
>>>> "somewhat frail" but nowhere near "incredibly frail".  The technique
>>>> has served us well for many years now.
>>>>
>>>> If you are driving javac through the Compiler API, you can register
>>>> a DiagnosticListener, and verify the characteristics of the Diagnostic
>>>> objects passed to report.
>>>>
>>>> -- Jon
>>>>
>>>>
>>>> On 06/14/2013 01:14 PM, Eric McCorkle wrote:
>>>>> Is there a more convenient API for checking error messages?  Golden
>>>>> will
>>>>> make for an incredibly frail test, I think.
>>>>>
>>>>> On 06/14/13 15:46, Jonathan Gibbons wrote:
>>>>>> The change to Gen looks OK, but the tests look weak.  At a minimum, I
>>>>>> would expect to see the test check the validity of the error message
>>>>>> that is generated; even better would be to generate the test cases on
>>>>>> the fly.
>>>>>>
>>>>>> -- Jon
>>>>>>
>>>>>> On 06/14/2013 11:29 AM, Eric McCorkle wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review this patch, which addresses a problem with javac not
>>>>>>> taking inner this parameters into account when determining if
>>>>>>> there are
>>>>>>> too many parameters to a function.
>>>>>>>
>>>>>>> The webrev is here:
>>>>>>> http://cr.openjdk.java.net/~emc/8014230/
>>>>>>>
>>>>>>> The bug report is here:
>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8014230
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Eric
>>>>