RFR 8189708: class reading issue with named and annotated parameters
Vicente Romero
vicente.romero at oracle.com
Fri Dec 15 15:08:08 UTC 2017
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>
More information about the compiler-dev
mailing list