JDK-8147527: Wrong code generated for postfix unary operators

[B. Blaser]

Hi,

2016-12-05 7:49 GMT+01:00 Jan Lahoda <jan.lahoda at oracle.com>:
> Hi,
>
> Thanks for your cvomments. Updated webrev:
> http://cr.openjdk.java.net/~jlahoda/8147527/webrev.02/
>
> I changed the second comment to:
> //"tmp1" and "tmp2" may refer to the same instance
> //(for e.g. <Class>.super.<ident>). But further processing may
> //change the components of the tree in place (see visitSelect),
> //so cloning the tree to avoid interference between the two uses:
>
> and added your testcase.
>
Wonderful!

> Any comments are welcome!
>
> Jan
>
Ok for me,
Bernard

>
> On 3.12.2016 13:07, B. Blaser wrote:
>>
>> Hi Jan,
>>
>> 2016-12-02 12:22 GMT+01:00 Jan Lahoda <jan.lahoda at oracle.com>:
>>>
>>> Hi,
>>>
>>> I've added these comments:
>>> //Need to use the "lhs" at two places, once on the future left hand side
>>> //and once in the future binary operator. But further processing may
>>> change
>>> //the components of the tree in place (see visitSelect for e.g.
>>> <Class>.super.<ident>),
>>> //so cloning the tree to avoid interference between the uses:
>>>
>> Marvellous!
>>
>>> //"tmp1" and "tmp2" may be the same instance
>>> //(for e.g. <Class>.super.<ident>). But further processing may
>>> //change the components of the tree in place (see visitSelect),
>>> //so cloning the tree to avoid interference between the two uses:
>>>
>> Ok ("tmp2 may refer to the instance held in tmp1", to be exact).
>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~jlahoda/8147527/webrev.01/
>>>
>>> How does this look?
>>>
>> Perfect to me.
>>
>> I've also noticed that the desugared test probably misses a "qualified
>> this" example (C.this.i++).
>> This isn't really necessary but almost interesting as it involves the
>> correct handling of "this$n" without any accessor generation (in
>> opposition to "C.super.i++" which needs them), as shown below.
>>
>>> Thanks!
>>>
>>> Jan
>>>
>> That's all, thank you!
>>
>> Bernard
>>
>> diff -u test/tools/javac/desugar/BoxingAndSuper.java.rev01
>> test/tools/javac/desugar/BoxingAndSuper.java
>> --- test/tools/javac/desugar/BoxingAndSuper.java.rev01    2016-12-03
>> 10:45:22.803874916 +0100
>> +++ test/tools/javac/desugar/BoxingAndSuper.java    2016-12-03
>> 12:12:16.246333070 +0100
>> @@ -204,6 +204,25 @@
>>                   "    (let /*synthetic*/ final Integer $le2 =
>> (Integer)this.i in (let /*synthetic*/ final Integer $le3 = this.i =
>> Integer.valueOf((int)(this.i.intValue() + 1)) in $le2));\n" +
>>                   "}\n";
>>           runTest(code, expected);
>> +        //qualified this:
>> +        runTest("public class Test {\n" +
>> +                "   Integer i;\n" +
>> +                "   class Inner1 {\n" +
>> +                "       class Inner2 {\n" +
>> +                "           private Integer dump() {\n" +
>> +                "               return Test.this.i++;\n" +
>> +                "           }\n" +
>> +                "       }\n" +
>> +                "   }\n" +
>> +                "}",
>> +                "Test.Inner1.Inner2.dump()java.lang.Integer\n" +
>> +                "{\n" +
>> +                "    return (let /*synthetic*/ final Integer $le0 =
>> (Integer)this$1.this$0.i" +
>> +                    " in (let /*synthetic*/ final Integer $le1 =
>> this$1.this$0.i = " +
>> +
>> "Integer.valueOf((int)(this$1.this$0.i.intValue() + 1))" +
>> +                        " in $le0));\n" +
>> +                "}\n"
>> +        );
>>       }
>>
>>       private final ToolBox tb = new ToolBox();
>>
>>>
>>> On 29.11.2016 22:07, B. Blaser wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> 2016-11-29 16:28 GMT+01:00 Maurizio Cimadamore
>>>> <maurizio.cimadamore at oracle.com>:
>>>>>
>>>>>
>>>>> Looks good to me. Use of shallow clone looks a tad odd - but I
>>>>> understand
>>>>> why it's the way it is - maybe worth adding a comment?
>>>>>
>>>>> Maurizio
>>>>
>>>>
>>>>
>>>> I agree that a short comment might be helpful, something like:
>>>>
>>>> // Shallow clone shall fit JDK-8147527 requirements, according to
>>>> related threads.
>>>>
>>>> Bernard
>>>>
>>>>> On 25/11/16 11:38, Jan Lahoda wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> As noted here:
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-October/010452.html
>>>>>>
>>>>>> javac sometimes generates code incorrectly, as discussed further in
>>>>>> the
>>>>>> thread.
>>>>>>
>>>>>> The proposed fix is to not reuse the AST nodes on multiple places.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8147527
>>>>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8147527/webrev.00/
>>>>>>
>>>>>> Any feedback is welcome.
>>>>>>
>>>>>> Thanks to Bernard for his work on this.
>>>>>>
>>>>>> Jan
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>