On Wed, 31 May 2023 05:42:58 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Classfile context object and multi-state options have been discussed at >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html >> This patch implements the proposed changes in Classfile API and fixes all >> affected code across JDK sources and tests. >> >> Please review. >> >> Thanks, >> Adam > > src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 87: > >> 85: * @param r a function mapping attribute names to attribute mappers >> 86: */ >> 87: record AttributeMapperOption(Function<Utf8Entry, AttributeMapper<?>> >> attributeMapper) implements Option { > > We might want to convert these options into interfaces and move the > implementation records into ClassfileImpl, like Brian suggested about > ClassHierarchyInfo. Will fix it, thanks. > src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 208: > >> 206: */ >> 207: default ClassModel parse(byte[] bytes) { >> 208: return new ClassImpl(bytes, (ClassfileImpl)this); > > Should we implement this in ClassfileImpl to avoid the redundant cast? yes, several methods should move to ClassfileImpl, thanks. > src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java > line 102: > >> 100: * @return re-mapped class file bytes >> 101: */ >> 102: default byte[] remapClass(ClassModel clm) { > > This api should ask for a Classfile object instead. Thankfully, the new > Classfile object actually allows us to find potentially bugs in our code (say > if the remapped class refer to declared types outside of system class loader) yes, I'll add the parameter, thanks. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212084790 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212094893 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212117128
