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

[Vicente Romero]

looks good, I will push it,

Thanks!
Vicente

On 02/04/2018 01:27 AM, Liam Miller-Cushon wrote:
> Thanks for the reminder, done:
> http://cr.openjdk.java.net/~cushon/8190452/webrev.03/ 
> <http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.03/>
>
> On Sat, Feb 3, 2018 at 8:59 PM, Jonathan Gibbons 
> <jonathan.gibbons at oracle.com <mailto:jonathan.gibbons at oracle.com>> wrote:
>
>     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/20180205/2b2d3e42/attachment-0001.html>