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

[Liam Miller-Cushon]

Thanks for the reminder, done:
http://cr.openjdk.java.net/~cushon/8190452/webrev.03/

On Sat, Feb 3, 2018 at 8:59 PM, Jonathan Gibbons <
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/
>
> On Sat, Feb 3, 2018 at 10:39 AM, Vicente Romero <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/
>>
>> > 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
>> > 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>
>>> 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-Jan
>>>> uary/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
>>>> webrev: http://cr.openjdk.java.net/~cushon/8190452/webrev.00/
>>>>
>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180203/6c102f11/attachment-0001.html>