Skip to content
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

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

Godin
Copy link
Member

@Godin Godin commented Feb 10, 2018

Fixes #640

@Godin Godin added this to the 0.8.1 milestone Feb 10, 2018
@Godin Godin self-assigned this Feb 10, 2018
@Godin Godin added this to IN PROGRESS in Filtering Feb 10, 2018
@Godin Godin force-pushed the issue-640 branch 5 times, most recently from 40e9811 to 161a8fc Compare February 10, 2018 23:50
@Godin Godin requested a review from marchof February 11, 2018 00:10
@Godin
Copy link
Member Author

Godin commented Feb 11, 2018

@marchof could you please review?

@Godin Godin force-pushed the issue-640 branch 2 times, most recently from 750e9ab to 47a1e13 Compare February 11, 2018 03:25
Copy link
Member

@marchof marchof left a 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);
Copy link
Member

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) {
Copy link
Member

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.

Copy link
Member Author

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 {
Copy link
Member

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.

Copy link
Member Author

@Godin Godin Feb 12, 2018

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@marchof marchof merged commit 06e8f36 into master Feb 12, 2018
@marchof marchof deleted the issue-640 branch February 12, 2018 12:45
@Godin Godin moved this from IN PROGRESS to DONE in Filtering Feb 12, 2018
@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Filtering
  
Done
Development

Successfully merging this pull request may close these issues.

Unnecessary empty Enum Contructor shows up on report
2 participants