RFR 8189708: class reading issue with named and annotated parameters

[Liam Miller-Cushon]

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>
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/
>
> 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> 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>> wrote:
>>>
>>>     Here's an updated patch that incorporates your approach:
>>>     http://cr.openjdk.java.net/~cushon/8007720/webrev.01/
>>>     <http://cr.openjdk.java.net/~cushon/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>> 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>> 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/fa6df99e/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8007720.patch
Type: application/octet-stream
Size: 26094 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20171214/fa6df99e/8007720-0001.patch>