RFR: 7903711: Rewrite simple methods ANC plugin to classfile API [v2]

[Leonid Kuskov]

On Fri, 12 Apr 2024 17:41:02 GMT, Alexandre Iline <shurailine at openjdk.org> wrote:

>> Rewritten using JDK 22.
>> 
>> openjdk.jcov.filter.simplemethods.InstructionIterator make the code a lot simpler.
>> 
>> No change needed to tests, except to change from ASM ClassNode to ClassModel from the classfile API
>
> Alexandre Iline has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Missing LDC instructions

Looks good to me, except cosmetic issues in code style.

plugins/simple_methods_anc/src/openjdk/jcov/filter/simplemethods/Delegators.java line 59:

> 57:             var iter = new InstructionIterator(m.code().get());
> 58:             var next = iter.next(i -> !isSimpleInstruction(i.opcode()));
> 59:             if (next == null || !isInvokeInstruction(next.opcode())) return false;

Bad codestyle - the line should be splitted into 2 ones.

if (next == null || !isInvokeInstruction(next.opcode())) 
    return false;

plugins/simple_methods_anc/src/openjdk/jcov/filter/simplemethods/Delegators.java line 64:

> 62:                 if (next instanceof InvokeInstruction ii) name = ii.name().toString();
> 63:                 else if (next instanceof InvokeDynamicInstruction idi) name = idi.name().toString();
> 64:                 else throw new IllegalStateException(STR."Unknown node type: \{next.getClass().getName()}");

hmm - it's more readable

String name = switch(next) {
    InvokeInstruction ii ->  ii.name().toString();
    InvokeDynamicInstruction idi -> idi.name().toString();
    default ->  throw new IllegalStateException(STR."Unknown node type: {next.getClass().getName()}");
}

plugins/simple_methods_anc/src/openjdk/jcov/filter/simplemethods/Delegators.java line 65:

> 63:                 else if (next instanceof InvokeDynamicInstruction idi) name = idi.name().toString();
> 64:                 else throw new IllegalStateException(STR."Unknown node type: \{next.getClass().getName()}");
> 65:                 if (!m.methodName().toString().equals(name)) return false;

first comment

plugins/simple_methods_anc/src/openjdk/jcov/filter/simplemethods/EmptyMethods.java line 38:

> 36:     public boolean test(ClassModel node, MethodModel m) {
> 37:         if (m.code().isPresent()) {
> 38:             var next = new InstructionIterator(m.code().get()).next(i -> !isSimpleInstruction(i.opcode()));

Bad practice to use var in "shared" projects except tests - a human isn't javac, for readability it would be much better to see a type of the result.

-------------

PR Comment: https://git.openjdk.org/jcov/pull/44#issuecomment-2052544351
PR Review Comment: https://git.openjdk.org/jcov/pull/44#discussion_r1563191638
PR Review Comment: https://git.openjdk.org/jcov/pull/44#discussion_r1563194325
PR Review Comment: https://git.openjdk.org/jcov/pull/44#discussion_r1563194768
PR Review Comment: https://git.openjdk.org/jcov/pull/44#discussion_r1563199316