On Mon, 31 Aug 2026 15:10:16 GMT, Viktor Klang <[email protected]> wrote:
>> 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.
>
> 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?
There's a little bit of tension here between the API and the implementation
class. The issue here in JsonArrayImpl is that the constructors take whatever
List is given and rely on the callers to provide a List that this class can
own, that contains no nulls, and that's not going to be modified, so wrapping
in an unmodifiable wrapper is ok. It would make this class more clear if the
constructor(s) were to use `List.copyOf` and then asList() could simply return
the already-unmodifiable List.
However, the public API call `JsonArray.of()` itself calls `List.copyOf()` on
its argument before passing it in, which potentially makes a copy, but its main
effect is to throw NPE if any element is null. It could be removed, which would
create a reliance of the API method on one of the internal constructors here.
Or `List.copyOf` could be called in both places, which seems redundant, but in
fact it won't create a redundant copy.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3898657394
