On Thu, 6 Jun 2024 18:35:01 GMT, Chen Liang <li...@openjdk.org> wrote:
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Looks good to me. Probably should get someone to OK changes in foreign/abi (@JornVernee perhaps?). There are some pre-existing places where we call `ReferenceClassDescImpl.ofValidated` directly that could probably be switched over to `ConstantUtils.classDesc` for slightly nicer code. Or - if it matters - add a `referenceClassDesc` which avoids the `type.isPrimitive` test. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 256: > 254: // the field name holding the method handle for this method > 255: String fieldName = "m" + count++; > 256: var mt = methodDesc(m.getReturnType(), > JLRA.getExecutableSharedParameterTypes(m)); Suggestion: var md = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m)); src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 469: > 467: // o instanceof Wrapped(float) > 468: cb.aload(SELECTOR_OBJ); > 469: > cb.instanceOf(classDesc(Wrapper.forBasicType(classLabel) I have a patch somewhere to cache the wrapper class desc in `sun.invoke.util.Wrapper`, both as a micro-optimization and to help disambigutate the unfortunately named (my bad) `Wrapper::classDescriptor` method. Maybe we can roll that into this..? src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 471: > 469: case Allocate allocate -> > emitAllocBuffer(allocate); > 470: case BoxAddress boxAddress -> > emitBoxAddress(boxAddress); > 471: case SegmentBase _ -> emitSegmentBase(); Seem a bit too far detached from the changes at hand for a drive-by code cleanup? ------------- Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19585#pullrequestreview-2102953208 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630049028 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630061889 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630074187