On Tue, 1 Sep 2026 04:35:32 GMT, Stuart Marks <[email protected]> wrote:
>> Naoto Sato has updated the pull request incrementally with nine additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'jdk-sandbox/json' into >> JDK-8381976-Implementation-for-Simple-JSON-API >> - offset -> cursorPos in JsonParser >> - Consolidated copyOf() in JsonArrayImpl constructor >> - Make Utils.composeTypeError derive expected from Class name >> - Refactor Frame classes in JG into records >> - Give initial size to StringBuilder >> - initSb() -> StringBuilder::new >> - Making Util class final >> - Clarify JSON string -> text in toDisplayString > > src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonValueSupport.java > line 46: > >> 44: */ >> 45: int offset(); >> 46: } > > This is kind of a general comment, but I'll put it here. JsonValueSupport > isn't much of an abstraction. It's a way to extract some commonality across > the various JsonValue implementations when they have a "doc" and an "offset". > But there's no abstraction here. The doc might be null if there's no > underlying doc... except for JsonStringImpl, which has a "doc" but it might > be from the JsonString.of() method instead of from parse(), so it's not > really a doc, so for that case we have a separate boolean that makes sure not > to return the doc if it came from a factory instead of from parse. > > What will put some pressure on this is if we eventually parse JSON from a > byte array (which we likely presume to be encoded in UTF-8) and so the > location will have a byte array and a byte offset. > > I think what's necessary at some point -- not immediately -- is a better > abstraction for the document position of a JsonValue, and have it be an > actual object. Then have a "null" object whose semantics are "no location" > instead of passing and checking for (null, -1) in various places. And put the > generation of the location information for an error message into that object > instead of having disembodied logic in the Util class. Agree. The current one is based solely on `char[]`. We will need a better abstraction of underlying document. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3906135261
