RFR 8189708: class reading issue with named and annotated parameters

[Vicente Romero]

looks good to me, only one minor comment, consider using a more 
meaningful name for:

test/langtools/tools/javac/MethodParameters/8189708/Test.java

there is no need for another review iteration for this change,

Thanks,
Vicente

On 12/13/2017 09:02 PM, Liam Miller-Cushon wrote:
> Thanks,
>
> I updated the copyright header and moved the tests to the correct 
> location:
> http://cr.openjdk.java.net/~cushon/8007720/webrev.02/ 
> <http://cr.openjdk.java.net/%7Ecushon/8007720/webrev.02/>
>
> Also - note that the patch currently includes a fix for JDK-8177486 as 
> well, since you suggested I add test coverage for enum and inner class 
> constructors, and mandated parameters aren't handled correctly without 
> fixing the other bug. Is it OK to have one changeset for both issues, 
> or should they be separated out? I could remove the enum and inner 
> class test coverage and the fix, and then add it back in as a 
> follow-up change if you prefer.
>
> On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <jan.lahoda at oracle.com 
> <mailto:jan.lahoda at oracle.com>> wrote:
>
>     Sorry for late reply.
>
>     Seems OK to me, only:
>     -the tests should be in:
>     test/langtools/tools/javac/MethodParameters/8189708
>     not in:
>     test/tools/javac/MethodParameters/8189708
>
>     -test/tools/javac/MethodParameters/8189708/Test.java has a weird
>     license header
>
>     Vicente, could you please take a look as well?
>
>     Thanks,
>         Jan
>
>     On 4.12.2017 22:37, Liam Miller-Cushon wrote:
>
>         Hi, is there any more feedback on this?
>
>         On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon
>         <cushon at google.com <mailto:cushon at google.com>
>         <mailto:cushon at google.com <mailto:cushon at google.com>>> wrote:
>
>             Here's an updated patch that incorporates your approach:
>         http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
>         <http://cr.openjdk.java.net/%7Ecushon/8007720/webrev.01/>
>             <http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
>         <http://cr.openjdk.java.net/%7Ecushon/8007720/webrev.01/>>
>
>             I included the fix for JDK-8177486 so I could test the
>         inner class /
>             enum constructor case. If this looks like it's on the
>         right track
>             I'll move that part (and the corresponding tests) back into a
>             separate change.
>
>             On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon
>             <cushon at google.com <mailto:cushon at google.com>
>         <mailto:cushon at google.com <mailto:cushon at google.com>>> wrote:
>
>                 Thanks for the comments,
>
>                 On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda
>                 <jan.lahoda at oracle.com <mailto:jan.lahoda at oracle.com>
>         <mailto:jan.lahoda at oracle.com <mailto:jan.lahoda at oracle.com>>>
>         wrote:
>
>                     -what happens if there are both runtime invisible and
>                     visible annotations of method's parameters? Seems
>         those that
>                     appear later will overwrite those that appear sooner?
>
>
>                 Oops, thanks. The way your patch handles that looks
>         good to me.
>
>                     -the MethodSymbol.savedParameterAnnotations is
>         only used
>                     during reading inside the ClassReader, right? It seems
>                     wasteful to have it as a field on each
>         MethodSymbol, better
>                     would be a field in ClassReader.
>
>
>                 Sounds good. I'll try to avoid having
>         savedParameterNames as a
>                 field in MethodSymbol also. Do you remember if you
>         encountered
>                 any issues with that in your patch?
>
>                     -please check what happens for annotations on
>         constructors
>                     of enums/non-static innerclasses
>
>
>                 Will do. (Also, note that there appears to be an issue
>         with
>                 reading MethodParameters on constructors of
>         enums/non-static
>                 inner classes: JDK-8177486)
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20171214/1b71e78e/attachment.html>