On Mon, 10 Aug 2026 20:47:55 GMT, Chen Liang <[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)) >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > src/jdk.incubator.json/share/classes/jdk/incubator/json/Json.java line 130: > >> 128: case JsonObject jo -> toDisplayString(jo, s, col, indent, >> isField); >> 129: case JsonArray ja -> toDisplayString(ja, s, col, indent, >> isField); >> 130: default -> s.append(" ".repeat(isField ? 1 : >> col)).append(jv); > > Suggestion: > > default -> s.repeat(' ', isField ? 1 : col).append(jv); This may change since we are considering to take indent as a String. > src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonArray.java line > 78: > >> 76: .stream() >> 77: .map(Objects::requireNonNull) >> 78: .collect(Collectors.toCollection(ArrayList::new)) > > Suggestion: > > .collect(Collectors.toList()) > > Or you can use `src.stream().map(Objects::requireNonNull).toList()` - this > list does not NPE upon `contains(null)` but is immutable. Suggested one seems fine, but the other one (Stream.toList()) won't compile, as it produces List<? extends JsonValue) > src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java > line 117: > >> 115: // Check for empty case >> 116: if (charEquals('}')) { >> 117: return new JsonObjectImpl(Map.of(), startO, doc); > > `Map.of` NPEs on `containsKey` and `containsValue`, not sure if you desire > this behavior given the unmodifiable-wrapped linked hash map doesn't have > this behavior. I think this is OK, as we don't define the behavior of the returned map from JsonObject.asMap() > src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java > line 222: > >> 220: // Check for empty case >> 221: if (charEquals(']')) { >> 222: return new JsonArrayImpl(List.of(), startO, doc); > > Similar observation for `List.of()` versus unmodifiable-wrapped `ArrayList` > as for `Map.of()` and unmodifiable-wrapped `LinkedHashMap`. same here ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3760525795 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3760631924 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3760709935 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3760711872
