Repository navigation
Skip integer conversion in accept()/SAX validation when the value is unused - #5484
Merged
Merged
Conversation
Owner
Author
|
Verified with a local benchmark (
The integer-array speedup matches the PR's intent: skipping — posted by Claude Code on behalf of @nlohmann |
gregmarr
reviewed
Sep 5, 2026
…unused lexer::scan_number() always converted every numeric token with strtoull()/strtoll() before returning, even though accept() (and any consumer using json_sax_acceptor) immediately discards the converted value. For value_unsigned/value_integer tokens whose digit count already guarantees the value fits into 64 bits, the conversion cannot change the accept/reject decision (such tokens are always finite and unconditionally accepted), so scan_number() can skip strtoull()/ strtoll() entirely in that case when the caller signals it does not need the value. Numbers with more digits keep using the exact, unmodified conversion path, so overflow reclassification to value_float (and the finiteness check on it) is unaffected. parse() and value_float handling are completely unchanged. Fixes #5411 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
…nsigned_t/number_integer_t width Signed-off-by: Niels Lohmann <mail@nlohmann.me>
…ferential test Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
force-pushed
the
issue-5411-lexer-skip-conversion
branch
from
September 8, 2026 11:12
68c6981 to
03270c6
Compare
gregmarr
approved these changes
Sep 8, 2026
7 tasks done
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
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
Fixes #5411.
lexer::scan_number()eagerly converts every numeric token withstrtoull()/strtoll()/strtof()at the end of scanning, even though foraccept()(and any SAX consumer that discards the numeric value, i.e.json_sax_acceptor) that converted value is never used. The only thing the conversion still affects foraccept()/reject is the parser's non-finite check onvalue_floattokens.This PR threads a
discard_number_valuesflag frombasic_json::accept()down throughparser/lexer. When set,lexer::scan_number()can skipstrtoull()/strtoll()entirely forvalue_unsigned/value_integertokens once the digit count alone guarantees the value fits into 64 bits (a decimal number with up to 18 digits always fits into bothstd::int64_tandstd::uint64_t, soerrnocould never have been set toERANGE). Such tokens are, by construction, always finite and unconditionally accepted regardless of their actual value, so only the token classification is needed — not the converted value.Numbers with 19+ digits (rare in practice) fall through to the exact, completely unmodified conversion code, so their handling — including reclassification to
value_floatwhen the value overflows 64 bits, and the existing406"number overflow" rejection when that reclassified value is not even finite as adouble— is bit-for-bit identical to before this change.Scope and what was deliberately left out
The originating issue's "suggested direction" also proposed (2) a cheap digit-count-based reclassification check to fully replace
strtoull/strtolleven near the 64-bit boundary, and (3) a cheap decimal-magnitude finiteness check to replacestrtofforvalue_floattokens. Both are out of scope for this PR:value_floathandling is completely unchanged — the existingstrtof/strtod/strtoldconversion is still always called, and the parser'sstd::isfinitecheck on it is untouched.value_unsigned/value_integer, the fast path is only taken when the digit count makes 64-bit overflow provably impossible (≤18 digits); everything else (≥19 digits) uses the exact original code path, unchanged. I judged a full digit-count-based reclassification of the ≥19-digit boundary cases to be higher risk for a security-sensitive parser without more extensive validation than fits this PR, so I scoped this down to the always-safe subset.parse()(and the publicsax_parse()API for user-supplied SAX consumers) are completely unaffected — they never setdiscard_number_values, so they keep calling the full conversion exactly as before.Behavior preservation / testing
accept("1e999") == false,accept("9999999999999999999999999999") == true(28-digit integer, overflowsuint64_tbut is finite asdouble).tests/src/unit-class_parser.cpp(SECTION("issue #5411 - skip conversion when accept() does not need the numeric value")) comparingjson::accept()againstjson::parse()across: small/large integers (both signs), the 18/19/20-digit boundary (both signs), the 64-bit boundaries (INT64_MAX/INT64_MIN/UINT64_MAX/UINT64_MAX+1), the 28-digit example from the issue, huge digit-only integers that overflow even adouble(must reject),1e999/1e400-style exponent overflow (must reject), values straddlingDBL_MAX, and a mix of valid/invalid numeric syntax.accept()output byte-for-byte between the pre-change and post-change headers (both the splitinclude/headers and the amalgamatedsingle_include/nlohmann/json.hpp) over ~2000 cases (hand-picked edge cases + randomized fuzz corpus of digit/exponent combinations): zero mismatches.json_test_datasubmodule available in this environment):unit-class_lexer.cpp,unit-class_parser.cpp,unit-deserialization.cpp,unit-regression1.cpp,unit-regression2.cpp,unit-udt.cpp,unit-class_parser_diagnostic_positions.cpp— all pass, with the exception of 7 pre-existing assertions inunit-regression1.cppthat fail identically ondevelopin this offline environment because they read fixture files from the (here, unavailable)json_test_datatest corpus; unrelated to this change (verified by running the identical test against unmodifieddevelopand getting the same 7 failures).make amalgamatewas run andsingle_include/nlohmann/json.hpp/json_fwd.hppare included in this PR.Breaking change?
No breaking changes. This only adds a new defaulted trailing parameter to the internal (
nlohmann::detail::parser/lexer, and theJSON_PRIVATE_UNLESS_TESTEDbasic_json::parser(...)factory) constructors; the publicbasic_json::accept()/parse()/sax_parse()API signatures are unchanged.— opened by Claude Code on behalf of @nlohmann