JDK-8147527: Wrong code generated for postfix unary operators

[B. Blaser]

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