Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 23:35:10 GMT, liach wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: >> >>> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >>> 968: String baseTypeString = wrapper.basicTypeString(); >>> 969: wrapperClassName = >>> dotToSlash(wrapper.wrapperType().getName()); >> >> Suggestion: >> >> wrapperClassName = wrapper.wrapperType().descriptorString(); >> >> >> It may worth to replace similar use of `dotToSlash(c.getName())` pattern. > > Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor > (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor > construction. ah, you're right. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:55:02 GMT, Mandy Chung wrote: >> liach 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 three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > Have you considered including the opcode for load and return instruction > instead of `opcodeOffset`? Hi @mlchung Mandy, I've changed the offset to seperate load and return opcodes, which is more in-line with how other fields in `PrimitiveTypeInfo` are used. In addition, I specified that the `wrapperClassName` is an internal name for clarification. Would you mind review this again, and review #8800, which avoids eager class initialization for parameter types used by proxy? - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:51:39 GMT, Mandy Chung wrote: >> liach 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 three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > >> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >> 968: String baseTypeString = wrapper.basicTypeString(); >> 969: wrapperClassName = >> dotToSlash(wrapper.wrapperType().getName()); > > Suggestion: > > wrapperClassName = wrapper.wrapperType().descriptorString(); > > > It may worth to replace similar use of `dotToSlash(c.getName())` pattern. Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor construction. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 20:55:53 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach 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 three additional commits since > the last revision: > > - Merge branch 'master' into proxy-primitivetypeinfo > - Convert PrimitiveTypeInfo to an enum > - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo Have you considered including the opcode for load and return instruction instead of `opcodeOffset`? src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > 967: // single-char BaseType descriptor (see JVMS section 4.3.2) > 968: String baseTypeString = wrapper.basicTypeString(); > 969: wrapperClassName = > dotToSlash(wrapper.wrapperType().getName()); Suggestion: wrapperClassName = wrapper.wrapperType().descriptorString(); It may worth to replace similar use of `dotToSlash(c.getName())` pattern. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 986: > 984: if (cl == float.class) return FLOAT; > 985: if (cl == double.class) return DOUBLE; > 986: throw new AssertionError(); Suggestion: throw new AssertionError(cl); - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the > hash map with a simple lookup, similar to what's done in > [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) liach 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 three additional commits since the last revision: - Merge branch 'master' into proxy-primitivetypeinfo - Convert PrimitiveTypeInfo to an enum - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/be9987bb..96c0835e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8801=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8801=01-02 Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 mod Patch: https://git.openjdk.java.net/jdk/pull/8801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801 PR: https://git.openjdk.java.net/jdk/pull/8801
