If nextIs() would have an boolean return value, we could write this as:

return nextIs(Opcodes.Return)

Could also be used in other existing filters.

@marchof then there will be temptation to use it with && everywhere, but that is exactly what we were trying to avoid.

@Godin It's internal API, I would try to keep the filter as readable as possible.

@marchof Then in case of preceding if statement containing return of a boolean, IDE start to suggest to combine them, making code unreadable. Also even if I don't see this in IntelliJ, static analyzers might warn about unused return value in other places.

The trick with ASTORE is to define a named argument. This looks surprising as there is no ASTORE involved in private empty constructors.

For better documentation I would therefore prefer to encapsulate this within AbstractMatcher and provide a method for this:

void defineLocal(int idx, String name)

@marchof I removed usage of nextIsVar - it does not work for a first instruction in absence of preceding label.

@Godin Ok, I see. I would still prefer to implement semantical matching premitives in AbstractMatcher, so we can write e.g.

	private boolean match(final MethodNode methodNode,
			final String superClassName) {
		cursor = methodNode.instructions.getFirst();
		skipNonOpcodes();
		nextIsVar(Opcodes.ALOAD, 0);
		nextIsMethod(Opcodes.INVOKESPECIAL, superClassName, "<init>",
				"()V");
		return nextIs(Opcodes.RETURN);
	}

[Updated]

@marchof I added skipNonOpcodes, but nextIsVar anyway can't be used for a very first instruction, because after skipNonOpcodes we need to check current instruction, while semantic of next... methods is to move to a next instruction. Change of semantic to check of current instruction - first of all will affect a lot of existing code relying on it, but anyway will lead to the same problem, a kind of, but at the end of method. IMO the only way - is to insert an artificial label at the beginning of all methods, but IMO this is an overkill compared just to a condition in if here, which is IMO perfectly readable.

Replacement of another if on method nextIsMethod in abstract class IMO also an overkill, because its only usage will be here, and there won't be code-sharing with other places where we already check MethodInsnNode (see references on it in TryWithResourcesJavacFilter and TryWithResourcesEcjFilter), and again IMO condition is perfectly readable.

@Godin Thanks for the explations. This would require to keep a state in AbstractMatcher and replace the direct access to cursor with simething like start(AbstractInsnNode).

But I agree that refactoring the matcher API soes not provide enough value for now.

@Godin I wonder whether we should add another negative test for the case where the private non-arg constructor is not empty in source code:

private static class PrivateNonEmptyNoArg {
	private PrivateNonEmptyNoArg() {
		nop(); // $line-privateNonEmptyNoArg$
	}
}

Done

@Godin Thx! 👍