[GitHub] [groovy] paulk-asert commented on a change in pull request #1361: GROOVY-9732: SC: re-implement transformExpression and several setters in CompareToNullExpression

[GitBox <gi...@apache.org>]

paulk-asert commented on a change in pull request #1361:
URL: https://github.com/apache/groovy/pull/1361#discussion_r487374598



##########
File path: src/main/java/org/codehaus/groovy/transform/sc/transformers/CompareToNullExpression.java
##########
@@ -31,25 +30,47 @@
 import org.codehaus.groovy.syntax.Types;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
-import org.objectweb.asm.Opcodes;
 
-public class CompareToNullExpression extends BinaryExpression implements Opcodes {
+import static org.objectweb.asm.Opcodes.GOTO;
+import static org.objectweb.asm.Opcodes.ICONST_0;
+import static org.objectweb.asm.Opcodes.ICONST_1;
+import static org.objectweb.asm.Opcodes.IFNONNULL;
+import static org.objectweb.asm.Opcodes.IFNULL;
+
+public class CompareToNullExpression extends BinaryExpression {

Review comment:
       Removing interface `Opcodes` is a binary breaking change. I agree it is ugly to leak that implementation class out through our API and acknowledge that style of usage dates back to before Java had static imports. Never-the-less, I think we should make such a change as a separate issue and target just Groovy 4. I'll create such an issue and merge this one without that change.




----------------------------------------------------------------
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:
users@infra.apache.org