Resolve the TODO(niels) in get_ubjson_string - #5355
Open
nlohmann wants to merge 1 commit into
Open
Conversation
nlohmann
added
aspect: binary formats
The comment asked whether the no-op marker 'N' may be ignored when a string is read. It may not: at that point the next byte must be a string length type specification, and 'N' is not one. No-ops at positions where a value may start are already consumed by the callers through get_ignore_noop(), so nothing is lost by not skipping them here. Replace the TODO with a comment stating that, and add regression tests pinning both directions: a no-op is accepted at top level (also repeated), before and after an array element, and before an object key, between key and value, and before the closing brace of an object of unknown size; it is rejected where a length type specification is expected, i.e. after the 'S' marker of a string value and as the key length of an object of known size. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann
force-pushed
the
claude/todo-243-ubjson-noop-todo
branch
from
August 4, 2026 06:52
86b3d6c to
2b23d1b
Compare
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
1 participant
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
binary_reader::get_ubjson_stringcarried a decade-old open question:The answer is no, and the surrounding code already implements it correctly — it was just never written down or covered by a test.
Two reasons:
U,i,I,l,L). A no-op is not valid there, and a bareNin that position is genuinely malformed input that the reader correctly rejects withparse_error.113.parse_ubjson_internalgo throughget_ignore_noop(). The only caller that passesget_char = truefor an object key is the sized (optimized) object path, where UBJSON does not permit no-ops at all.So the
get()is right as it stands. This PR replaces the TODO with a comment saying why, and pins the behavior with regression tests.Changes
include/nlohmann/detail/input/binary_reader.hpp— TODO replaced by an explanatory comment. No behavior change; this is the only remainingTODOin the parsing headers.tests/src/unit-ubjson.cpp— newSECTION("no-op markers")underparsing values, 9 assertions:N i1→1;N N N i1→1;[N i1]→[1];[i1 N]→[1];{N U1"a" i1}→{"a":1};{U1"a" N i1}→{"a":1};{U1"a" i1 N}→{"a":1}parse_error.113("expected length type specification"):S N U1"a"(no-op after theSmarker) and{#i1 N U1"a" i1}(no-op as the key length of a sized object)single_include/nlohmann/json.hpp— regenerated viamake amalgamate.All of the above was verified against
developbefore writing the tests, and the fullUBJSONtest case passes locally (691 256 assertions).Breaking changes to the public API
None. The change is a comment plus tests; parser behavior, signatures, and ABI are untouched.
This pull request was prepared by Claude Code.