On Tue, 1 Sep 2026 16:21:49 GMT, Naoto Sato <[email protected]> wrote:
>> 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. Great. And just to be absolutely clear, I don't think this needs to be done before the initial integration. It will require a larger scale refactoring and will probably requires a few rounds of iteration, so it can be postponed until later, possibly even the next incubator. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3908160656
