[GitHub] [groovy] sonatype-lift[bot] commented on a change in pull request #1706: GROOVY-10540: add `GroovyObject` interface before STC and classgen

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

sonatype-lift[bot] commented on a change in pull request #1706:
URL: https://github.com/apache/groovy/pull/1706#discussion_r832627100



##########
File path: src/main/java/groovy/transform/stc/FromAbstractTypeMethods.java
##########
@@ -40,28 +42,15 @@
     @Override
     public List<ClassNode[]> getClosureSignatures(final MethodNode node, final SourceUnit sourceUnit, final CompilationUnit compilationUnit, final String[] options, final ASTNode usage) {
         String className = options[0];
-        ClassNode cn = findClassNode(sourceUnit, compilationUnit, className);
-        return extractSignaturesFromMethods(cn);
-    }
+        ClassNode classNode = findClassNode(sourceUnit, compilationUnit, className);
 
-    private static List<ClassNode[]> extractSignaturesFromMethods(final ClassNode cn) {
-        List<MethodNode> methods = cn.getAllDeclaredMethods();
-        List<ClassNode[]> signatures = new LinkedList<ClassNode[]>();
-        for (MethodNode method : methods) {
-            if (!method.isSynthetic() && method.isAbstract()) {
-                extractParametersFromMethod(signatures, method);
+        List<ClassNode[]> signatures = new LinkedList<>();

Review comment:
       *JdkObsolete:*  It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless you're willing to invest a lot of time into benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does not. [(details)](https://errorprone.info/bugpattern/JdkObsolete)
   ```suggestion
           List<ClassNode[]> signatures = new ArrayList<>();
   ```
   
   
   
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)

##########
File path: src/main/java/org/codehaus/groovy/ast/ClassHelper.java
##########
@@ -562,52 +560,45 @@ public static boolean isGeneratedFunction(final ClassNode type) {
      * @return the method node if type is a SAM type, null otherwise
      */
     public static MethodNode findSAM(final ClassNode type) {
-        if (!Modifier.isAbstract(type.getModifiers())) return null;
         if (type.isInterface()) {
-            List<MethodNode> methods;
-            if (type.isInterface()) {
-                // e.g. BinaryOperator extends BiFunction, BinaryOperator contains no abstract method, but it is really a SAM
-                methods = type.redirect().getAllDeclaredMethods();
-            } else {
-                methods = type.getMethods();
-            }
-
-            MethodNode found = null;
-            for (MethodNode mi : methods) {
-                // ignore methods, that are not abstract and from Object
-                if (!Modifier.isAbstract(mi.getModifiers())) continue;
-                // ignore trait methods which have a default implementation
-                if (Traits.hasDefaultImplementation(mi)) continue;
-                if (isObjectType(mi.getDeclaringClass())) continue;
-                if (OBJECT_TYPE.getDeclaredMethod(mi.getName(), mi.getParameters()) != null) continue;
+            MethodNode sam = null;
+            for (MethodNode mn : type.getAbstractMethods()) {
+                // ignore methods that will have an implementation
+                if (Traits.hasDefaultImplementation(mn)) continue;
+                if (OBJECT_TYPE.getDeclaredMethod(mn.getName(), mn.getParameters()) != null) continue;
 
                 // we have two methods, so no SAM
-                if (found != null) return null;
-                found = mi;
+                if (sam != null) return null;
+                sam = mn;
             }
-            return found;
-
-        } else {
-            MethodNode found = null;
-            for (MethodNode mi : type.getAbstractMethods()) {
-                if (!hasUsableImplementation(type, mi)) {
-                    if (found != null) return null;
-                    found = mi;
+            return sam;
+        }
+        if (type.isAbstract()) {
+            MethodNode sam = null;
+            for (MethodNode mn : type.getAbstractMethods()) {
+                if (!hasUsableImplementation(type, mn)) {
+                    if (sam != null) return null;
+                    sam = mn;
                 }
             }
-            return found;
+            return sam;
         }
+        return null;
     }
 
-    private static boolean hasUsableImplementation(ClassNode c, MethodNode m) {
-        if (c == m.getDeclaringClass()) return false;
+    private static boolean hasUsableImplementation(final ClassNode c, final MethodNode m) {
+        ClassNode declaringClass = m.getDeclaringClass();
+        if (c == declaringClass) return false;

Review comment:
       *ReferenceEquality:*  Comparison using reference equality instead of value equality [(details)](https://errorprone.info/bugpattern/ReferenceEquality)
   ```suggestion
           if (Objects.equals(c, declaringClass)) return false;' or 'if (c.equals(declaringClass)) return false;
   ```
   
   
   
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)




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

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org