On Mon, 31 Aug 2026 22:02:52 GMT, Naoto Sato <[email protected]> wrote:
>> 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. Definitely out of scope and probably obvious, but if there ever was something like `Map::ofSequenced` / `Map::ofEntriesSequenced` / `Map::copyOfSequenced`, `JsonObjectImpl` would be a prime candidate for the latter. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3899502457
