On Fri, 3 Feb 2023 15:11:30 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes and this patch converts it to use Classfile API. >> >> This pull request suppose to chain on the 8294982: Implementation of >> Classfile API https://github.com/openjdk/jdk/pull/10982 >> >> Please review. >> >> Thank you, >> Adam > > 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 four additional > commits since the last revision: > > - j.l.r.ProxyGenerator fix - Classfile API moved under > jdk.internal.classfile package > - Merge branch 'JDK-8294982' into JDK-8294961 > - Merge branch 'JDK-8294982' into JDK-8294961 > - 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate > proxy classes This looks good in general. I like this, easy to use, easy to read and understand and greatly simplified. Reviewing this is one easy way to study the ClassFile API. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 390: > 388: uniqueList.add(ex); > 389: } > 390: return uniqueList.stream().map(ex -> > ClassDesc.ofDescriptor(ex.descriptorString())).toList(); It would be useful to add a helper method to convert Class to ClassDesc: private static ClassDesc toClassDesc(Class<?> type) { return ClassDesc.ofDescriptor(type.descriptorString()); } Suggestion: return uniqueList.stream().map(ProxyGenerator::toClassDesc).toList(); src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 423: > 421: private byte[] generateClassFile() { > 422: var localCache = new HashMap<ClassDesc, > ClassHierarchyResolver.ClassHierarchyInfo>(); > 423: return Classfile.build(classDesc, > List.of(Classfile.Option.classHierarchyResolver(classDesc -> How/when is `ClassHierarchyResolver` being called? I didn't dig deeper. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 664: > 662: */ > 663: private void generateMethod(ClassBuilder clb, ClassDesc > className) { > 664: MethodTypeDesc desc = > MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, > parameterTypes).toMethodDescriptorString()); I slightly prefer to call `MethodType::descriptorString` that `descriptorString` is called for both `Class` and `MethodType`. MethodTypeDesc desc = MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, parameterTypes).descriptorString()); Alternatively, use `describeConstable` instead. MethodTypeDesc desc = MethodType.methodType(returnType, parameterTypes).describeConstable().orElseThrow(); src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 665: > 663: private void generateMethod(ClassBuilder clb, ClassDesc > className) { > 664: MethodTypeDesc desc = > MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, > parameterTypes).toMethodDescriptorString()); > 665: int accessFlags = (method.isVarArgs()) ? ACC_VARARGS | > ACC_PUBLIC | ACC_FINAL : ACC_PUBLIC | ACC_FINAL; Nit: Suggestion: int accessFlags = (method.isVarArgs()) ? ACC_VARARGS | ACC_PUBLIC | ACC_FINAL : ACC_PUBLIC | ACC_FINAL; src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 672: > 670: .stream() > 671: .map(cpb::classEntry) > 672: .toList(); Suggestion: List<ClassEntry> exceptionClassEntries = Arrays.asList(exceptionTypes) .stream() .map(ProxyGenerator::toClassDesc) .map(cpb::classEntry) .toList(); `typeNames` streams over the given types. This can be simplified by mapping `Class` to `ClassDesc` in place here. I would also suggest to add the `toClassDesc` helper method. ------------- PR: https://git.openjdk.org/jdk/pull/10991
