Repository navigation
Share DOM SAX position handling; fix stale parser and lexer comments - #5731
Merged
Merged
Conversation
json_sax_dom_parser and json_sax_dom_callback_parser each had a private copy of handle_diagnostic_positions_for_json_value(), identical except for comments. Move the body into one static member function, detail::diagnostic_positions::set_from_lexer(value, lexer), which both classes call with their lexer pointer. basic_json befriends the new struct (only when JSON_DIAGNOSTIC_POSITIONS is enabled), as the position members are private. The discarded case is reached through the callback parser, so the LCOV_EXCL markers that only the dom parser's copy had are gone. The NOLINT on the unreachable default case loses the stray "-warnings-as-errors", which is not a check name. The start-position setup in start_object()/start_array() is left alone, as #5706 is editing the callback parser's versions. Behavior, the public API and the ABI are unchanged. Part of #5712 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The class documentation called the parser a recursive descent parser, but sax_parse_internal() is a loop that keeps the open containers on an explicit stack. The comment at the end of an array and of an object said the flag is set to false while the code below it sets it to true. Describe what the code does instead. Comments only; behavior, the public API and the ABI are unchanged. Part of #5712 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The comments explaining the accept() shortcut in convert_number() and the member documentation still argued in terms of strtoull()/strtoll() and errno, which #5283 replaced with convert_integer(), and pointed at scan_number() instead of convert_number(). They also did not say that scan_number_bulk_contiguous() converts integers itself, so the shortcut is only reached for input without bulk access, with JSON_DIAGNOSTIC_POSITIONS, or when the bulk scanner falls back. Rewrite both comments to describe the digit-count check in front of convert_integer(), keeping the 18-digit bound and the json_sax_acceptor argument. The stale <cstdlib> comment is left for after #5616, which edits that include block. Comments only; behavior, the public API and the ABI are unchanged. Part of #5712 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The documentation of decode() called the Hoehrmann DFA the single source of truth for UTF-8 validation. It is used only by the serializer and by is_valid_utf8() (CBOR/MessagePack/BSON/UBJSON/BJData text strings). The lexer's scan_string() switch, validate_one_utf8() / valid_utf8_prefix() (bulk string scan, BON8 bulk path and BON8 writer) and the BON8 byte path in get_bon8_string() check the RFC 3629 ranges on their own. Replace the sentence with a list of the four validators, what each is used for, and a note that they must accept the same sequences. Sharing code between them was considered and dropped: it would save a few lines in a validator that is entangled with BON8 pushback, and #5677 is editing the BON8 byte path. Comments only; behavior, the public API and the ABI are unchanged. Part of #5712 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
input_adapters.hpp included <memory> and <numeric> for the removed shared_ptr-based adapter design but used neither; it called (std::min) without including <algorithm>. json_sax.hpp used std::numeric_limits without including <limits>. Also corrected comments that no longer matched the code: input_stream_adapter does not skip the input's BOM (the lexer's skip_bom() does), the span_input_adapter comment named the no-longer-existing input_buffer_adapter type, lexer::get_string() does not reset the token, binary_reader's get_number() doc opened with /* instead of /*! (so Doxygen skipped it) and omitted BON8 from its endianness note, and the UBJSON-binary-types note did not mention that BJData 'B' arrays are read as binary. Left out: the lgtm suppression on lexer.hpp's scan_number() (in #5616's hunk) and the "-1 if unknown" wording in json_sax.hpp's start_object/start_array docs (in draft #5267's hunk), per the verdict's conflict list. Part of #5712 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
…arse() json_sax_dom_callback_parser and json_sax_dom_parser branches of parser::parse() ran the same ~25 lines after sax_parse_internal(): the strict-mode EOF check (raising parse_error.101 through the SAX parser), release_lookahead() in non-strict mode, and mapping an errored SAX parser to a discarded result. The two copies had already drifted apart in formatting and in the second copy's "see above" comment. Add a private parse_dom(DomSax&, strict) member that runs this shared sequence once and returns whether the SAX parser did not error; both branches of parse() now only construct their DOM SAX parser, call parse_dom(), and (for the callback parser) map a discarded top-level value to null. sax_parse() is left untouched, since it only runs the EOF check and release_lookahead() when sax_parse_internal() succeeded, unlike parse(), which runs them unconditionally. Behavior-preserving: same operations in the same order for both SAX parser kinds. Verified with unit-class_parser (strict/non-strict, callback and non-callback), unit-deserialization and unit-disabled_exceptions (JSON_NOEXCEPTION), plus a clean make amalgamate / make check-amalgamation diff. Overlaps #5601, which touches the same lines. Signed-off-by: Niels Lohmann <mail@nlohmann.me> #5712 item 2
…s and the lexer The 1/2/3/4-byte UTF-8 encoding ladder was written out by hand three times: in wide_string_input_helper<..., 4>::fill_buffer() for a UTF-32 code point, in the UTF-16 helper for both a BMP code unit and a valid surrogate pair, and in the lexer's \uXXXX/\uXXXX\uYYYY handling. The copies had drifted: the UTF-32 helper masked the leading bits of each byte (& 0x1Fu, & 0x0Fu, & 0x07u) where the others relied on the shift alone, even though both give the same result for a code point that is already known to be in range. Add detail::encode_utf8(cp, out) in string_utils.hpp, a single encoder that invokes a callable once per output byte, most significant byte first. Use it in the three valid-code-point branches (UTF-32 code points up to U+10FFFF, UTF-16 code units outside the surrogate range, and valid UTF-16 surrogate pairs) and in the lexer's \u handling, where out forwards to add(). The UTF-16 helper's deliberate pass-through of malformed surrogate units and the UTF-32 helper's 0xFF sentinel for code points above U+10FFFF are untouched, since neither reaches the new helper. Behavior-preserving: same bytes in the same order for every valid code point, verified with unit-class_lexer, unit-class_parser, unit-deserialization, unit-wstring and the non-test-data parts of unit-unicode1..5 (ASan/UBSan, C++11/17/20), and an escape-heavy parse microbenchmark that shows no change (about 73 ms either way, median of 3, 1M escape sequences). single_include/ regenerated with make amalgamate; make check-amalgamation leaves a clean tree. Overlaps #5704, which rewrites the wide_string_input_helper specializations touched here. Signed-off-by: Niels Lohmann <mail@nlohmann.me> #5712 item 6
nlohmann
added
the
review needed
gregmarr
reviewed
Sep 30, 2026
|
|
||
| #if JSON_DIAGNOSTIC_POSITIONS | ||
| handle_diagnostic_positions_for_json_value(ref_stack.back()->m_data.m_value.array->back()); | ||
| diagnostic_positions::set_from_lexer(ref_stack.back()->m_data.m_value.array->back(), m_lexer_ref); |
Contributor
There was a problem hiding this comment.
Should these calls all check m_lexer_ref != nullptr up front so they don't have to dig into stacks and such if they aren't going to do anything? That then removes a level of nesting in set_from_lexer.
Contributor
There was a problem hiding this comment.
It's a bit weird that m_lexer_ref is a pointer, but that's not the fault of this PR.
gregmarr
approved these changes
Sep 30, 2026
nlohmann
removed
the
review needed
github-actions
Bot
added
aspect: binary formats
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the "do-now" items of the technical-debt umbrella issue #5712: deduplicating the diagnostic-position setter shared by the two DOM SAX parsers, and fixing a set of stale/incorrect comments and include lists in the parser, lexer and input headers. Each commit is one checklist item of #5712 and can be reviewed on its own. All changes are behavior-preserving; two commits are code changes (deduplication), the rest are comment-only or include-only fixes. The follow-up commits listed under "Remaining checklist items" implement every item that was left open, so this PR now closes #5712; see "Public API" for their effects.
Changes
handle_diagnostic_positions_for_json_value()bodies ofjson_sax_dom_parserandjson_sax_dom_callback_parserinto one static member function,detail::diagnostic_positions::set_from_lexer(value, lexer), called by both classes; drops the staleLCOV_EXCLmarkers on thediscardedcase (reachable from the callback parser) and the invalid-warnings-as-errorsNOLINT check name. (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 1)parser.hppclass documentation ("recursive descent parser" → iterative parser with an explicit stack) and theskip_to_state_evaluationcomments at the end of array/object parsing, which described the flag being set to the opposite of what the code does. (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 3)discard_number_valuescomments inconvert_number()and the member documentation to describe the current digit-count check in front ofconvert_integer(), replacing references to thestrtoull()/strtoll()path removed by Speed up parsing of contiguous input (numbers, strings, UTF-8) #5283, and note that the shortcut is only reached on the non-bulk path. (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 4)decode()documentation's claim that the Hoehrmann DFA is "the single source of truth for UTF-8 validation" with a list of the four validators actually in use (the DFA/is_valid_utf8(), the lexer'sscan_string()switch,validate_one_utf8()/valid_utf8_prefix(), and the BON8 byte path), noting they must accept the same sequences. (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 5)detail/input/headers: remove unused<memory>/<numeric>and add<algorithm>(for(std::min)) ininput_adapters.hpp, add<limits>tojson_sax.hpp, and correct comments that no longer matched the code (BOM handling belongs to the lexer, not the stream adapter; a reference to the removedinput_buffer_adaptertype; theget_number()Doxygen comment marker and its endianness note, including BON8's big-endian byte order; and the UBJSON/BJData binary-type note). (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 7)Tests
Verified locally per commit:
make amalgamate/make check-amalgamationproduced a clean diff on every commit (amalgamate +make prettywith astyle 3.4.13,BUILD.bazelunchanged). For the position-deduplication commit,unit-diagnostic-positionsandunit-class_parser(including withJSON_DIAGNOSTIC_POSITIONS=1, which exercises the callback parser'sdiscardedpath formerly excluded from coverage) passed, along withunit-alt-string,unit-ordered_json,unit-allocator,unit-diagnostics,unit-deserialization(JSON_USE_IMPLICIT_CONVERSIONS=0) andunit-disabled_exceptions(JSON_NOEXCEPTION), built with ASan/UBSan under clang++ and g++-16 at-std=c++11, with no new warnings under-Wall -Wextra -Wshadoworclang -Weverything. For the include-hygiene commit, each changed header (input_adapters.hpp,json_sax.hpp,lexer.hpp,binary_reader.hpp) was compiled standalone with-fsyntax-only(clang++,-std=c++11) to confirm the corrected include lists are self-sufficient, andunit-deserialization,unit-class_lexer,unit-cborandunit-bon8were run (ASan/UBSan), including a targeted re-check of the BON8 endianness note againstwrite_bon8_integer()'s own documentation and an empiricalto_bon8()byte-order check. A standalone build/run smoke test was also done for every commit that touchesinclude/, confirming the branch builds correctly at each step, not just at the tip.CI must confirm:
ci_clang_tidyandcppcheckcheck-amalgamationjob on the final PR statePublic API
No breaking changes. All five commits are documented as behavior-preserving: items 1's deduplication only moves an identical private helper's body behind a new
detail::diagnostic_positionsstruct thatbasic_jsonbefriends (underJSON_DIAGNOSTIC_POSITIONS) and removes stale coverage/lint annotations; the remaining four commits change only comments and internal include lists. No public-namespace names are added or removed, no observable behavior or compile errors change, and the ABI is unaffected.Follow-up commits:
Remaining checklist items (follow-up commits)
Items 2 and 6, which were left open because they overlap open PRs, are now implemented here as well, so this PR closes #5712.
32d5540d3Deduplicate the strict-EOF /release_lookahead()/ error block of the twoparser::parse()branches intoparse_dom();sax_parse()is unchanged (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 2)4356d0fe5Share the code point to UTF-8 encoding between the UTF-16/UTF-32wide_string_input_helpers and the lexer's\uhandling viadetail::encode_utf8(); the malformed-surrogate passthrough and the UTF-32 sentinel are unchanged (JSON parser, lexer and SAX DOM parsers: duplicated position/EOF/UTF-8 encoding code and stale comments and includes #5712 item 6)Tests for these commits:
unit-class_parser,unit-class_lexer,unit-deserialization,unit-wstring,unit-diagnostic-positions,unit-precise-stream-position(withJSON_PRECISE_STREAM_POSITION) andunit-disabled_exceptions(withJSON_NOEXCEPTION), with ASan/UBSan, C++11/17/20make check-amalgamationcleanItem 2 overlaps draft #5601, item 6 overlaps #5704. Whichever PR lands second needs a rebase.
Closes #5712
This PR was written by Claude Code on behalf of @nlohmann.
🤖 Generated with Claude Code