On Wed, 3 Jan 2024 12:36:26 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > StackCounter fix I think it's okay to split the fix to SplitConstantPool.java and StackCounter.java in a separate PR now. This PR can go next once reviewed. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 28: > 26: package java.lang.reflect; > 27: > 28: import java.lang.classfile.*; Nit: sort the imports in alphabetical orders. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 112: > 110: private static final ClassModel TEMPLATE; > 111: > 112: private static final Utf8Entry UE_Method; Nit: good to group this list of static variables by type for netter readability. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 135: > 133: var cc = ClassFile.of(); > 134: var entries = new ArrayList<PoolEntry>(20); > 135: var q = new Object() { It'd be helpful to add a comment to explain what this optimization is doing. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 148: > 146: generateLookupAccessor(clb); > 147: var cp = clb.constantPool(); > 148: q.entries = new PoolEntry[] { line 174-197 must match the order of CP entries being added. Is there other way to avoid `q.next()` coupling with the order of CP entries in `q.entries`? Maybe define constants for the indices to `q.entries` such that writing and reading the CP entries from the array using the constant index is easier to read and less error-prone? src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 299: > 297: /** > 298: * {@return the entries of the given type} > 299: * @param type the {@code Class} objects, no primitives nor arrays Suggestion: * @param types the {@code Class} objects, not primitive types nor array types src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 303: > 301: private static ClassEntry[] toClassEntries(ConstantPoolBuilder cp, > List<Class<?>> types) { > 302: var ces = new ClassEntry[types.size()]; > 303: for (int i = 0; i< ces.length; i++) Suggestion: for (int i = 0; i < ces.length; i++) src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 739: > 737: var desc = new StringJoiner("", "(", ")" + > returnType.descriptorString()); > 738: for (var pt : parameterTypes) { > 739: desc.add(pt.descriptorString()); Maybe worth refactor these as `ProxyMethod::toMethodTypeDescriptorString` with the comment to explain why `MethodType::descriptorString` is not used. ------------- PR Review: https://git.openjdk.org/jdk/pull/17121#pullrequestreview-1806808189 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443351490 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443352842 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443355763 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443365641 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443371360 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443370121 PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1443381525
