On Mon, 5 Jun 2023 14:42:32 GMT, Adam Sotona <asot...@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 > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed copyright header Overall this looks pretty clean. I like how simply it flows through the implementation classes. There are a number of methods and classes, especially in Classfile, which need some more spec. src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 50: > 48: /** > 49: * Main entry points for parsing, transforming, and generating classfiles. > 50: */ Now that this is not just a bag of static methods any more, we need to have some spec text about what a Classfile is. Something like: Represents a context for parsing, transforming, and generating classfiles. A `Classfile` has a set of options that condition how parsing and generation is done. src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 54: > 52: permits ClassfileImpl { > 53: > 54: static Classfile of() { Classfile::of needs specification src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 63: > 61: > 62: Classfile withOptions(Option... options); > 63: Classfile::withOptions needs specification src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 66: > 64: /** > 65: * An option that affects the writing of classfiles. > 66: */ Options affect parsing as well as writing now src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java line 82: > 80: static ConstantPoolBuilder of() { > 81: return new SplitConstantPool(); > 82: } The old ctors here were (ClassModel) and (Options), where the ClassModel version had a trail of breadcrumbs back to options. The new ctors are (ClassModel) and no-args, which is a slight departure. What use did CPB make of options in the first place, if any? ------------- PR Review: https://git.openjdk.org/jdk/pull/14180#pullrequestreview-1462788383 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1218222043 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1218223437 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1218223102 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1218223991 PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1218316864
