RFR: JDK-8190452: javac should not add MethodParameters attributes to v51 and earlier class files

[Jonathan Gibbons]

Liam,

 From a minor stylistic point of view, we generally place the test 
description (the comment beginning @test) after the legal header, and 
before any code like package and import statements

-- Jon

On 2/3/18 6:27 PM, Liam Miller-Cushon wrote:
> I reworked the test to use class reading, and added the -Xlint:option 
> warning.
>
> Here's the updated patch: 
> http://cr.openjdk.java.net/~cushon/8190452/webrev.02/ 
> <http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.02/>
>
> On Sat, Feb 3, 2018 at 10:39 AM, Vicente Romero 
> <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>
>
>
>     On 02/03/2018 12:37 AM, Liam Miller-Cushon wrote:
>>     Hi Vicente, thanks for the review!
>>
>>     I inlined the golden files into the test class. I don't think
>>     inlining Lib.java works with the current approach of using
>>     `@compile --release 7/8` and reflection to access the parameter
>>     names, because the reflective APIs were added in 8. I could
>>     rewrite the test to use class reading instead of reflection if
>>     you prefer.
>
>     yes I was thinking about class reading but I'm OK with the current
>     version of the patch
>
>>
>>     Here's the updated webrev:
>>     http://cr.openjdk.java.net/~cushon/8190452/webrev.01/
>>     <http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.01/>
>>
>>     > Regarding your questions about what to do with the -Xlint:X
>>     options. I don't have any opinion on one way or the other, is
>>     there any reason to change them?
>>
>>     I thought it was potentially surprising that -parameters will now
>>     be silently ignored for Java 7 and earlier language levels.
>>     Warning about that flag combination would make the new behaviour
>>     more discoverable.
>>
>>     Since the bug shipped in 9 and 10 there are some <v52 class files
>>     with MethodParameters in the wild, and I've seen cases where it
>>     broke builds using -Xlint:classfile and -Werror.
>>
>>     I don't think either of those are common problems. If you think
>>     we should leave the -Xlint handling as-is that sounds good to me.
>
>     as discussed off-line with Jon, yes please add the warning, but
>     feel free to do it as part of this bug or in a different one.
>
>     Vicente
>
>>
>>     On Fri, Feb 2, 2018 at 6:17 AM, Vicente Romero
>>     <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>>
>>         Hi Liam,
>>
>>         The fix looks OK, regarding the test, I don't see the need
>>         for the two golden files as they can be constants in the test
>>         per se. In addition, the whole test could be self contained
>>         in only one class that compiles the Lib.java source.
>>         Regarding your questions about what to do with the -Xlint:X
>>         options. I don't have any opinion on one way or the other, is
>>         there any reason to change them?
>>
>>         Thanks,
>>         Vicente
>>
>>
>>         On 02/01/2018 01:56 PM, Liam Miller-Cushon wrote:
>>>         Bump. I'm happy to implement either of the alternatives I
>>>         mentioned, but was hoping to get feedback on the approach first.
>>>
>>>         On Wed, Jan 17, 2018 at 11:51 AM, Liam Miller-Cushon
>>>         <cushon at google.com <mailto:cushon at google.com>> wrote:
>>>
>>>             Please review a fix for JDK-8190452. The change causes
>>>             javac to not emit MethodParameters attributes when
>>>             targeting v51 class files.
>>>
>>>             The change implements the suggestion from this thread:
>>>             http://mail.openjdk.java.net/pipermail/compiler-dev/2018-January/011579.html
>>>             <http://mail.openjdk.java.net/pipermail/compiler-dev/2018-January/011579.html>
>>>
>>>             There are two related changes that may be worth considering:
>>>             * now that -parameters will be ignored when compiling
>>>             with --release < 8, should this combination of flags
>>>             result in a warning if -Xlint:options is enabled?
>>>             * since this wasn't fixed in JDK 9, there are v51 class
>>>             files in the wild that contain unexpected
>>>             MethodParameters attributes. Should -Xlint:classfile be
>>>             relaxed to avoid warning on those?
>>>
>>>             bug: https://bugs.openjdk.java.net/browse/JDK-8190452
>>>             <https://bugs.openjdk.java.net/browse/JDK-8190452>
>>>             webrev:
>>>             http://cr.openjdk.java.net/~cushon/8190452/webrev.00/
>>>             <http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.00/>
>>>
>>>
>>
>>
>
>

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