[GitHub] [groovy] Fiouz opened a new pull request #1455: GROOVY-9892: fix operand stack for ++x / x++ in static compilation

[GitBox]

Fiouz opened a new pull request #1455:
URL: https://github.com/apache/groovy/pull/1455


   40ee3fd0c19530b4308c7c1b3e30abd4d45ed54a replaced an invocation of
   `ScriptBytecodeAdapter.setGroovyObjectProperty()` into an invocation of
   the generated `pfaccess$0*` mutator method.
   Contrary to `setGroovyObjectProperty()`, the mutator method has a return
   value, which ends up pushed to the operand stack.
   
   This commit discards the value being pushed to the stack by pop'ing it.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danielsun1106 commented on a change in pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#discussion_r557435563



##########
File path: src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy9892Bug.groovy
##########
@@ -0,0 +1,21 @@
+package org.codehaus.groovy.classgen.asm.sc.bugs

Review comment:
       license header is expected here:
   https://github.com/apache/groovy/blob/494c7ec11799c7b6c91f6d7689470544800b7416/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java#L1-L18




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danielsun1106 commented on a change in pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#discussion_r557436347



##########
File path: src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy9892Bug.groovy
##########
@@ -0,0 +1,21 @@
+package org.codehaus.groovy.classgen.asm.sc.bugs
+
+import groovy.transform.stc.StaticTypeCheckingTestCase
+import org.codehaus.groovy.classgen.asm.sc.StaticCompilationTestSupport
+
+class Groovy9892Bug extends StaticTypeCheckingTestCase implements StaticCompilationTestSupport {
+    void testUnaryOperationInGString() {
+        assertScript '''
+            class OuterClass {
+                int counter
+            
+                def call() {
+                    { ->
+                        "Hello${++counter}World"
+                    }.call()
+                }
+            }
+            assert new OuterClass().call() == 'Hello1World'

Review comment:
       Could you add a test case to cover `counter++` ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danielsun1106 commented on pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#issuecomment-760233974


   LGTM


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

Fiouz commented on a change in pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#discussion_r557566729



##########
File path: src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy9892Bug.groovy
##########
@@ -0,0 +1,21 @@
+package org.codehaus.groovy.classgen.asm.sc.bugs

Review comment:
       License header is now added




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

Fiouz commented on a change in pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#discussion_r557568194



##########
File path: src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy9892Bug.groovy
##########
@@ -0,0 +1,21 @@
+package org.codehaus.groovy.classgen.asm.sc.bugs
+
+import groovy.transform.stc.StaticTypeCheckingTestCase
+import org.codehaus.groovy.classgen.asm.sc.StaticCompilationTestSupport
+
+class Groovy9892Bug extends StaticTypeCheckingTestCase implements StaticCompilationTestSupport {
+    void testUnaryOperationInGString() {
+        assertScript '''
+            class OuterClass {
+                int counter
+            
+                def call() {
+                    { ->
+                        "Hello${++counter}World"
+                    }.call()
+                }
+            }
+            assert new OuterClass().call() == 'Hello1World'

Review comment:
       Test modified to verify both prefix and postfix increment




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

asfgit closed pull request #1455:
URL: https://github.com/apache/groovy/pull/1455


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danielsun1106 commented on pull request #1455:
URL: https://github.com/apache/groovy/pull/1455#issuecomment-760954203


   Merged. Thanks!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danielsun1106 merged pull request #1455:
URL: https://github.com/apache/groovy/pull/1455


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]