On Thu, 12 Oct 2023 11:11:10 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> removed obsolete exports from BuildMicrobenchmark.gmk > > src/java.base/share/classes/java/lang/classfile/package-info.java line 259: > >> 257: * the classfile building process. >> 258: * <p> >> 259: * Syntax validation is applied during symbolic descriptors >> construction. > > I would drop `syntax` - and just use `Validation` Yes, applied, thanks. > src/java.base/share/classes/java/lang/classfile/package-info.java line 265: > >> 263: * <p> >> 264: * Using symbolic descriptors assures the right serial form selection by >> 265: * the ClassFile API library. For example a class name is stored in its >> internal > > I find this comment still very hard to parse. There is a lot of terminology > here thrown to the reader (internal form, class descriptor, serial form), and > very little explanation of which is which. I get the general idea that, if I > use a `ClassDesc`, the API will take care to convert that to the right form, > depending on the context - but it feels there's too much text, and too few > examples to ground that text. I've tried to rephrase the paragraph and simplify it, thanks. > src/java.base/share/classes/java/lang/classfile/package-info.java line 278: > >> 276: * constant pool entries to avoid any form of syntax validation or >> conversion. >> 277: * Constant pool entries can be constructed from raw values, with no >> additional >> 278: * syntactic checks, conversions or validations. In the following >> example is > > `syntactic checks` and `validation` feel the same thing. If there's some > validation that is materially different from a syntactic check that is > important for the reader to grasp, then spell it out. Otherwise leave it out > and remain more general. Right, fixed, thanks. > src/java.base/share/classes/java/lang/classfile/package-info.java line 401: > >> 399: * >> 400: * <h3>Transformation handling of unknown classfile elements</h3> >> 401: * Future JDK releases may introduce new classfile elements, not known >> at the > > Maybe this is more direct/punchy? > > `Custom classfile transformations might be unaware of classfile elements > introduced by future JDK releases.` More clear, thanks. > src/java.base/share/classes/java/lang/classfile/package-info.java line 403: > >> 401: * Future JDK releases may introduce new classfile elements, not known >> at the >> 402: * development time of a custom transformation code. >> 403: * To achieve deterministic stability of transformations running on >> future JDK > > This para seems largely redundant. Perhaps you can merge with the next? E.g. > "To achieve deterministic stability, classfile transforms interested ..." Yes, merged according to the suggestion. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989724 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989504 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989983 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356990585 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356991173
