On Wed, 26 Aug 2026 23:52:23 GMT, Naoto Sato <[email protected]> wrote:
>> This PR implements [JEP 540: Simple JSON API >> (Incubator)](https://openjdk.org/jeps/540). >> >> It adds the `jdk.incubator.json` module which provides APIs for reading and >> writing JSON documents as specified by [RFC >> 8259](https://datatracker.ietf.org/doc/html/rfc8259). This is an incubating >> API. >> >> API documentation: >> https://cr.openjdk.org/~naoto/json/javadoc/api/jdk.incubator.json/module-summary.html >> Co-authored-by: Justin Lu >> ([@justin-curtis-lu](https://github.com/justin-curtis-lu)), Stuart Marks >> ([@stuart-marks](https://github.com/stuart-marks)) >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Naoto Sato has updated the pull request incrementally with five additional > commits since the last revision: > > - Merge remote-tracking branch 'jdk-sandbox/json' into > JDK-8381976-Implementation-for-Simple-JSON-API > - Yet another occurrence > - One more w.r.t previous commit > - Consistently refer to JSON text using double quotes > - Wording/formatting for JsonValue.asInt/asLong/asDouble. Did a brief pass over the code itself (not the tests). src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonArrayImpl.java line 58: > 56: public List<JsonValue> asList() { > 57: return Collections.unmodifiableList(theValues); > 58: } I wonder if it would make more sense to make the Impl-classes immutable (even if `doc` would by necessity be shallowly-immutable, subject to whether exposing the char-array really is the optimal thing), and ensuring that the List is an immutable one (which would then remove the need for wrapping with unmodifiableList). This could be enforced during the constructor, and if we can prove that construction will be with an ArrayList, then List.copyOf should attempt to avoid double-copying. If this is decided, then it would be preferable to clearly document that this class (and other similar impls) is immutable. Has it been considered to make these impl-classes `record`s? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonGenerator.java line 89: > 87: if (elements.hasNext()) { > 88: if (af.first) { > 89: af.first = false; You could likely make ArrayFrame and ObjectFrame immutable by instead of storing an Iterator and a mutable boolean, you store the collection, and then we post-add the newline/comma/delim and at the end we do a StringBuilder::setLength(currentLength - delim-length-to-delete-to-avoid-trailing-delim). Then ObjectFrame and ArrayFrame can both be records. src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 51: > 49: private final char[] doc; > 50: // Lazily initialized for member names with escape sequences > 51: private final LazyConstant<StringBuilder> sb = > LazyConstant.of(this::initSb); `StringBuilder::new`? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 53: > 51: private final LazyConstant<StringBuilder> sb = > LazyConstant.of(this::initSb); > 52: // Current offset during parsing > 53: private int offset; `offset` sounds static, but this is more like a cursor. Rename to something like `at` or `curPos` or something more clearly signalling that it is a moving target? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 44: > 42: * Shared utilities for Json classes. > 43: */ > 44: public class Utils { Suggestion: public final class Utils { src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 65: > 63: } else { > 64: if (sb == null) { > 65: sb = new StringBuilder().append(str, 0, i); Might make sense to pre-size that string-builder to the nearest power-of-two larger-or-equal to str.length()? (or at least to str.length()) to avoid having to do an immediate resize on the append-call. src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 97: > 95: > 96: // Use to compose an exception when casting to an incorrect type > 97: public static JsonValueException composeTypeError(JsonValue jv, > String expected) { `Class<? extends JsonValue> expected` would make this a bit less Stringly typed. ------------- PR Review: https://git.openjdk.org/jdk/pull/32282#pullrequestreview-5068043147 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3895710213 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3895822894 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3896324263 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3896220278 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3896589649 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3896606948 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3896624436
