RFR 8189708: class reading issue with named and annotated parameters

[Liam Miller-Cushon]

I'm not a committer yet, do you mind pushing it?

On Fri, Dec 15, 2017 at 10:08 AM, Vicente Romero <vicente.romero at oracle.com>
wrote:

> 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
> > 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/20171215/bd65f3ef/attachment-0001.html>