On Mon, 31 Aug 2026 21:45:16 GMT, Stuart Marks <[email protected]> wrote:
>> 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.
Thanks. Changed to use `List.copyOf()`, and consolidated the one in the factory
method into constructor. I did not do the same for `JsonObjectImpl`, as it
should preserve the encounter order of the backing map, which `Map.copyOf()`
does not guarantee.
As to the suggestion to change impls to records, we intentionally did not
define equality on `JsonValue` so that users would not accidentally count on
the behavior. Making the impls based on `record` may introduce that possibility.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3898783027
