JDK-8147527: Wrong code generated for postfix unary operators

[Jan Lahoda]

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.

Any comments are welcome!

Jan

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
>>>>
>>>>
>>>>
>>