On Tue, 16 Apr 2024 22:49:28 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 10 additional
>> commits since the last revision:
>>
>> - Reversion of ClassBuilder changes
>> - Merge branch 'master' into JDK-8294960-invoke
>> - Apply suggestions from code review
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>> - Apply suggestions from code review
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>> - added missing comment
>> - fixed InnerClassLambdaMetafactory for hidden caller classes
>> - performance improvements
>> - consolidation of descriptors handling
>> - InvokerBytecodeGenerator::emit... improvements
>> - 8294960: Convert java.base/java.lang.invoke package to use the Classfile
>> API to generate lambdas and method handlers
>
> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 32:
>
>> 30: import java.lang.classfile.attribute.SourceFileAttribute;
>> 31: import java.lang.constant.ClassDesc;
>> 32: import static java.lang.constant.ConstantDescs.*;
>
> Nit: this static imports can be moved to line 50 grouped with other static
> imports.
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 53:
>
>> 51: import static java.lang.invoke.MethodHandleStatics.*;
>> 52: import static java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP;
>> 53: import static java.lang.classfile.ClassFile.*;
>
> Nit: nice to sort these in alphabetical order - this before line 50.
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 606:
>
>> 604: TRANSFORM_MODS = List.of(tms.toArray(new Integer[0]));
>> 605: }
>> 606: private static final MethodTypeDesc MTD_transformHelper =
>> MethodTypeDesc.of(CD_MethodHandle, CD_int);
>
> Suggestion:
>
> private static final MethodTypeDesc MTD_TRANFORM_HELPER =
> MethodTypeDesc.of(CD_MethodHandle, CD_int);
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
> line 72:
>
>> 70: static final String INVOKERS_HOLDER_CLASS_NAME =
>> INVOKERS_HOLDER.replace('/', '.');
>> 71: static final String BMH_SPECIES_PREFIX =
>> "java.lang.invoke.BoundMethodHandle$Species_";
>> 72: private static final ClassFile CC = ClassFile.of();
>
> `ClassFile.of()` returns the ClassFile with default context which is a
> singleton object. Is this static variable needed?
Right, it is no more necessary.
> src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
> line 526:
>
>> 524: clb.withFlags(ACC_PRIVATE | ACC_FINAL | ACC_SUPER)
>> 525:
>> .withSuperclass(InvokerBytecodeGenerator.INVOKER_SUPER_DESC)
>> 526:
>> .with(SourceFileAttribute.of(clb.constantPool().utf8Entry(className.substring(className.lastIndexOf('/')
>> + 1))));
>
> Is it because of performance reason to use
> `SourceFileAttribute.of(Utf8Entry)` rather than
> `SourceFileAttribute.of(String)`? If so, a comment to explain would help.
It is redundant relic. Removed, thank you.
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 36:
>
>> 34: import java.lang.classfile.ClassBuilder;
>> 35: import java.lang.classfile.ClassFile;
>> 36: import static java.lang.classfile.ClassFile.*;
>
> Nit: group static imports in line 52 following this file convention.
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 41:
>
>> 39: import java.lang.constant.ClassDesc;
>> 40: import java.lang.constant.ConstantDesc;
>> 41: import static java.lang.constant.ConstantDescs.*;
>
> Nit: group static imports following this file convention
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 57:
>
>> 55: import java.lang.classfile.constantpool.FieldRefEntry;
>> 56: import java.lang.classfile.constantpool.InterfaceMethodRefEntry;
>> 57: import java.lang.classfile.constantpool.MethodRefEntry;
>
> Nit: group these imports with the above import java.lang.classfile...
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 28:
>
>> 26: package java.lang.invoke;
>> 27:
>> 28: import static java.lang.classfile.ClassFile.*;
>
> Nit: move this static import to line 64 following this file convention.
Fixed, thank you.
> src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java
> line 75:
>
>> 73: UNBOX_DOUBLE = unbox(CD_Number,
>> "doubleValue", CD_double);
>> 74:
>> 75: static private TypeKind primitiveTypeKindFromClass(Class<?> type) {
>
> Suggestion:
>
> private static TypeKind primitiveTypeKindFromClass(Class<?> type) {
Fixed, thank you.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024148
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024311
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024488
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569023878
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569023198
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024574
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024826
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024982
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569022391
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1569024668
