RFR 8189708: class reading issue with named and annotated parameters

[Vicente Romero]

Hi,

Looks good,
Vicente

PS. do you need me to push it or can you push already?

On 12/14/2017 06:55 PM, Liam Miller-Cushon wrote:
> Thanks for the reviews. I renamed the test, and mentioned both of the 
> associated bugs in the test and the commit message.
>
> The formatted changeset is attached.
>
> On Thu, Dec 14, 2017 at 9:28 AM, Vicente Romero 
> <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>
>     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/20171215/f5cec12c/attachment.html>