You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by paulk-asert <gi...@git.apache.org> on 2017/05/03 09:14:22 UTC
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
GitHub user paulk-asert opened a pull request:
https://github.com/apache/groovy/pull/533
GROOVY-7840: Verifier#makeDescriptorWithoutReturnType uses ClassNode#…
…toString with generics
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paulk-asert/groovy groovy7840
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/533.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #533
----
commit 8fc3816afaa475fa5ef3ad57a391dc08ef587274
Author: paulk <pa...@asert.com.au>
Date: 2017-05-03T09:12:31Z
GROOVY-7840: Verifier#makeDescriptorWithoutReturnType uses ClassNode#toString with generics
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116460943
--- Diff: src/main/org/apache/groovy/ast/tools/MethodNodeUtils.java ---
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+
+public class MethodNodeUtils {
+ /**
+ * Return the method node's descriptor including its
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's abbreviated descriptor excluding the return type
+ */
+ public static String methodDescriptorWithoutReturnType(MethodNode mNode) {
--- End diff --
I kept it distinct because I wasn't really sure that we should tie the particular formatting we have chosen to the more general method that some might expect to find in MethodNode. If you look at:
org.objectweb.asm.Type#getMethodDescriptor(java.lang.reflect.Method)
It would produce different output.
For example, for this method:
`int[] method(List arg1, String arg2)`
ASM would produce the familiar:
`(Ljava/util/List;Ljava/lang/String;)[I`
whereas we produce:
`[I method(java.util.List, java.lang.String)`
What we produce is suitable for our purposes (and we need the name) but I wonder what people would expect in MethodNode.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116533820
--- Diff: src/main/org/apache/groovy/ast/tools/MethodNodeUtils.java ---
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+
+public class MethodNodeUtils {
+ /**
+ * Return the method node's descriptor including its
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's abbreviated descriptor excluding the return type
+ */
+ public static String methodDescriptorWithoutReturnType(MethodNode mNode) {
--- End diff --
That is what is sort of confusing, the method descriptor for bytecode vs the method signature used to detect duplicate signatures. Agree that the formatting is particular to it's use in the `Verifier` and having in a separate utils class is probably the best approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116460996
--- Diff: src/main/org/apache/groovy/ast/tools/MethodNodeUtils.java ---
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+
+public class MethodNodeUtils {
+ /**
+ * Return the method node's descriptor including its
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's abbreviated descriptor excluding the return type
+ */
+ public static String methodDescriptorWithoutReturnType(MethodNode mNode) {
+ StringBuilder sb = new StringBuilder();
+ mNode.getTypeDescriptor();
+ sb.append(mNode.getName()).append(':');
+ for (Parameter p : mNode.getParameters()) {
+ sb.append(ClassNodeUtils.formatTypeName(p.getType())).append(',');
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Return the method node's descriptor which includes its return type,
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's descriptor
+ */
+ public static String methodDescriptor(MethodNode mNode) {
--- End diff --
See above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116363829
--- Diff: src/main/org/apache/groovy/ast/tools/MethodNodeUtils.java ---
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+
+public class MethodNodeUtils {
+ /**
+ * Return the method node's descriptor including its
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's abbreviated descriptor excluding the return type
+ */
+ public static String methodDescriptorWithoutReturnType(MethodNode mNode) {
+ StringBuilder sb = new StringBuilder();
+ mNode.getTypeDescriptor();
+ sb.append(mNode.getName()).append(':');
+ for (Parameter p : mNode.getParameters()) {
+ sb.append(ClassNodeUtils.formatTypeName(p.getType())).append(',');
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Return the method node's descriptor which includes its return type,
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's descriptor
+ */
+ public static String methodDescriptor(MethodNode mNode) {
--- End diff --
This could be the body of `MethodNode#getTypeDescriptor` instance method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116363767
--- Diff: src/main/org/apache/groovy/ast/tools/ClassNodeUtils.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.ClassNode;
+
+public class ClassNodeUtils {
+ /**
+ * Formats a type name into a human readable version. For arrays, appends "[]" to the formatted
+ * type name of the component. For unit class nodes, uses the class node name.
+ *
+ * @param cNode the type to format
+ * @return a human readable version of the type name (java.lang.String[] for example)
+ */
+ public static String formatTypeName(ClassNode cNode) {
--- End diff --
This seems like a candidate to be a package-private instance method on `ClassNode`. Package-private part assumes the moving of the `MethodNodeUtils` methods into `MethodNode`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/533#discussion_r116363909
--- Diff: src/main/org/apache/groovy/ast/tools/MethodNodeUtils.java ---
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ast.tools;
+
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+
+public class MethodNodeUtils {
+ /**
+ * Return the method node's descriptor including its
+ * name and parameter types without generics.
+ *
+ * @param mNode the method node
+ * @return the method node's abbreviated descriptor excluding the return type
+ */
+ public static String methodDescriptorWithoutReturnType(MethodNode mNode) {
--- End diff --
This seems like it could be an instance method such as `MethodNode#getSignature()` ([JLS 8.4.2](http://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.4.2)) or `MethodNode#getTypeDescriptorWithoutReturnType()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #533: GROOVY-7840: Verifier#makeDescriptorWithoutReturnT...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/533
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---