-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add filter for empty constructor without parameters in enum #649
Conversation
40e9811
to
161a8fc
Compare
@marchof could you please review? |
750e9ab
to
47a1e13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Thanks Evgeny! Looks good to me. I have some minor comments, but I'm also ok to merge as is.
private static class Matcher extends AbstractMatcher { | ||
private boolean match(final MethodNode methodNode, | ||
final String superClassName) { | ||
firstIsLoadThis(methodNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this very readable reader declaration ;)
* Sets {@link #cursor} to first instruction of method if it is | ||
* <code>ALOAD 0</code>, otherwise sets it to <code>null</code>. | ||
*/ | ||
void firstIsLoadThis(final MethodNode methodNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking the method name only applies to instance methods. For static methods it would be the first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof renamed into firstIsALoad0
.
@@ -17,6 +17,7 @@ | |||
import org.objectweb.asm.Opcodes; | |||
import org.objectweb.asm.tree.AbstractInsnNode; | |||
import org.objectweb.asm.tree.MethodInsnNode; | |||
import org.objectweb.asm.tree.MethodNode; | |||
import org.objectweb.asm.tree.VarInsnNode; | |||
|
|||
abstract class AbstractMatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it would be worth to add unit tests for AbstractMatcher. Especially as we miss several branches with the rest of our test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof Let's do this separately from this ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Fixes #640