GROOVY-7840: Verifier#makeDescriptorWithoutReturnType uses ClassNode#… #533
Conversation
jwagenleitner
left a comment
jwagenleitner
left a comment
There was a problem hiding this comment.
+1
To me the utility methods seem more like candidates to be instance methods, but overall the change looks good. If the util classes are kept it might be good to add private constructors.
| * @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) { |
There was a problem hiding this comment.
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.
| * @param mNode the method node | ||
| * @return the method node's descriptor | ||
| */ | ||
| public static String methodDescriptor(MethodNode mNode) { |
There was a problem hiding this comment.
This could be the body of MethodNode#getTypeDescriptor instance method.
| * @param mNode the method node | ||
| * @return the method node's abbreviated descriptor excluding the return type | ||
| */ | ||
| public static String methodDescriptorWithoutReturnType(MethodNode mNode) { |
There was a problem hiding this comment.
This seems like it could be an instance method such as MethodNode#getSignature() (JLS 8.4.2) or MethodNode#getTypeDescriptorWithoutReturnType().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I added an inline comment. It isn't clear-cut though, so let me know what you think. I have numerous other methods that would go in the respective util classes in another spike. But your right, if they stay a private constructor would be good. |
|
Thanks for the reply Paul. I see the point about the formatting and not wanting to make it part of the regular |
…toString with generics (closes apache#533)
…toString with generics (closes #533)
…toString with generics (closes #533)
3 participants
…toString with generics