Review request for 8055063: Parameter#toString() fails w/ AIOOBE for ctr of inner class w/ generic type

[Joel Borggrén-Franck]

Approved

thanks Eric,

cheers
/Joel

On 2014-11-10, Eric McCorkle wrote:
> Ok, the test has been revised as you suggested.
> 
> A new version is posted (webrev.03).
> 
> On 11/10/14 06:45, Joel Borggrén-Franck wrote:
> > Hi Eric,
> > 
> > The changes to Executable.java and Parameter.java looks good. The test
> > could still need some cleaning up:
> > 
> > @summary should be before "action tags", put it right after @bug
> > 
> > I don't think you need a separate file for InnerClassToStringNoParameters.java , should be able to accomplish the same with jtreg tags,
> > 
> >  * @clean InnerClassToString
> >  * @compile -parameters InnerClassToString.java
> >  * @run main InnerClassToString
> >  * @clean InnerClassToString
> >  * @compile InnerClassToString.java
> >  * @run main InnerClassToString
> > 
> > I think this is cleaner, YMMV.
> > 
> > The main method suffers from a lot of code duplication (and an absence
> > of line breaks), you can extract a test method since the test just
> > operates on two arrays of Type[] and call it with the parameter type
> > array and the answer array.
> > 
> > cheers
> > /Joel
> > 
> > On 2014-11-05, Eric McCorkle wrote:
> >> Thanks, Joel.  I've applied your fixes and re-upped the webrev.
> >>
> >> On 11/05/14 10:10, Joel Borggrén-Franck wrote:
> >>> Hi Eric,
> >>>
> >>> I think caching the real parameter type array on Executable is the wrong thing to do considering we can have a lot of instances of Executable but fairly few will be examined for parameters and the vast majority of those will never have this issue.
> >>>
> >>> Also, the tests are a bit minimal, I would expect you to check that the type of the parameter is correct. The tests should also have a @bug line.
> >>>
> >>> I think you are overly cautious here. I’m not sure we need to work around potential bugs in non-Java compilers. If language Y inserts synthetic/mandated parameters anywhere else than as a prefix they better generate a correct Signature (or non at all). This is a discussion we can defer however.
> >>>
> >>> If you flesh out the test and remove the caching I think this is a fine fix for both 8u and 9.
> >>>
> >>> cheers
> >>> /Joel
> >>>
> >>> On 31 okt 2014, at 17:15, Eric McCorkle <eric.mccorkle at oracle.com> wrote:
> >>>
> >>>> Hello,
> >>>>
> >>>> Please review this patch which fixes issues that arise with
> >>>> getGenericParameterTypes() and getAnnotatedParameterTypes() when there
> >>>> are generic signatures and synthetic parameters.
> >>>>
> >>>> Please note that a complete fix is not possible for all cases.  See
> >>>> discussion on https://bugs.openjdk.java.net/browse/JDK-8062582 for details.
> >>>>
> >>>> This patch will cause Executable.getAnnotatedParameterTypes(),
> >>>> Parameter.getAnnotatedType(), and Parameter.getParameterizedType() to
> >>>> report the correct types in the following cases:
> >>>>
> >>>> * No generic signature is present.
> >>>> * Both a generic signature and method parameters information are present
> >>>> * A generic signature is present, but method parameters information is
> >>>> not present, but the number of parameters in the generic signature and
> >>>> the number of parameters in the method descriptor are the same.
> >>>>
> >>>> In the problematic case, where there is a generic signature, no method
> >>>> parameters information, and the generic signature does not match the
> >>>> method descriptor, these methods will return the correct /non/-generic
> >>>> type, as there is no general way of associating parameters in the
> >>>> generic signature with those in the method descriptor in this case.
> >>>>
> >>>> Please also note that there is currently a bug in javac which causes
> >>>> type annotations' parameter indexes to be wrong when synthetic
> >>>> parameters are generated: https://bugs.openjdk.java.net/browse/JDK-8029012.
> >>>>
> >>>> The bug report is here:
> >>>> https://bugs.openjdk.java.net/browse/JDK-8055063
> >>>>
> >>>> The webrev is here:
> >>>> http://cr.openjdk.java.net/~emc/8055063/
> >>>
> > 
> >> begin:vcard
> >> fn:Eric McCorkle
> >> n:McCorkle;Eric
> >> org:Oracle Corporation;Java Platform Group
> >> adr:BUR02-2117;;35 Network Drive;Burlington;MA;01803;United States
> >> email;internet:eric.mccorkle at oracle.com
> >> title:Senior Member of Technical Staff
> >> tel;work:1-781-442-1028
> >> tel;cell:404-644-6360
> >> version:2.1
> >> end:vcard
> >>
> > 

> begin:vcard
> fn:Eric McCorkle
> n:McCorkle;Eric
> org:Oracle Corporation;Java Platform Group
> adr:BUR02-2117;;35 Network Drive;Burlington;MA;01803;United States
> email;internet:eric.mccorkle at oracle.com
> title:Senior Member of Technical Staff
> tel;work:1-781-442-1028
> tel;cell:404-644-6360
> version:2.1
> end:vcard
>