Avoid allocating temporary basic_json for cbor and msgpack object keys

[alexprabhat99]

This PR optimizes CBOR and MessagePack serialization by avoiding the construction of a temporary basic_json object for every object key during serialization.

Previously, object keys were passed through write_cbor()/write_msgpack(), which expected a BasicJsonType and therefore implicitly constructed a temporary basic_json for each key. This introduced unnecessary heap allocations during serialization.

This change factors out the string serialization logic into dedicated helper functions and serializes object keys directly. The implementation preserves compatibility with custom object key types while eliminating the extra basic_json allocations for the common case where object_t::key_type is string_t.

A regression test has been added to verify compatibility with custom object key types.

Fixes #5318

Compatibility note: This does not change the public API (detail::binary_writer is internal). For custom object_t::key_type implementations, CBOR and MessagePack object keys are now required to be implicitly convertible to string_t. Previously, a custom key type handled only through to_json could be serialized through the generic JSON serializer, potentially producing non-string map keys that the library's own CBOR/MessagePack readers would reject.

[alexprabhat99]

Hi @nlohmann
All checks are successful now.
Thanks!

[gregmarr]

Looks good to me.

[nlohmann]

Thanks for tackling this! Overall this looks good and does exactly what #5318 asked for. A few minor comments below.

The claimed win is real

I compiled the allocation-counting program from #5318 against single_include/nlohmann/json.hpp from develop and from this branch (clang, -std=c++14 -O2), 1000-key object:

case	develop	this PR
to_cbor, short (SSO) keys	1015	15
to_cbor, 64-char keys	2015	15
to_msgpack, short keys	1015	15
to_msgpack, 64-char keys	2015	15
to_ubjson / to_bson (unchanged)	16 / 14	16 / 14

CBOR and MessagePack now sit at the same allocation level as UBJSON and BSON. The extracted helper bodies are byte-for-byte the originals, and the value_t::string cases correctly reach the non-template overload via *j.m_data.m_value.string.

Comments

1. Missing SPDX header on the new test file

tests/src/custom_object_key_type.hpp starts straight at #pragma once. Every other file under tests/src carries the ASCII-art banner plus SPDX-FileCopyrightText / SPDX-License-Identifier from the json_support REUSE template. ci_reuse_compliance passes only because .reuse/dep5 has a Files: * catch-all, so CI can't catch this. Running make reuse should fix it.

2. key::c_str() in the test helper looks like dead code

It was added in d10514d ("fixes for failing ci"), but nothing on the CBOR/MessagePack path calls it — the template overload routes through string_t(value). I deleted it locally and the test still compiles and passes under -std=c++11/14/17/20/23. If the intent was UBJSON compatibility, the type would also need size(), which it doesn't have (binary_writer.hpp line 940 uses both), so to_ubjson wouldn't compile with this key type anyway.

3. The unconstrained template overloads may not be needed, and are undocumented

write_cbor_string(const KeyType&) and write_msgpack_string(const KeyType&). The BSON writer already solves the same problem by just declaring write_bson_element(const string_t& name, ...) and letting implicit conversion handle custom key types. I removed both templates from this branch's header and the new tests still compile and pass — the test's key has an implicit operator std::string(), so const string_t& binds directly.

So the templates' only added capability (key types with an explicit-only conversion to string_t) isn't exercised by the new tests, and they carry a small maintenance hazard: any future write_cbor_string(x) on a non-string_t silently allocates a temporary. Either drop them for consistency with the BSON writer, or keep them with a one-line comment explaining why — and make the test key's conversion operator explicit so the overload is actually load-bearing.

4. Worth noting the source-compatibility nuance in the PR description

Previously keys went through write_cbor() / write_msgpack(), i.e. through the JSON serializer. A key type with a to_json but no string conversion used to compile (emitting a non-string CBOR/MessagePack key, which this library's own reader then rejects); it now fails to compile. A key type with both a to_json and a string conversion changes its encoding.

I think the new behavior is the correct one, but since it's the convention here to state API impact: no public API break (detail::binary_writer is internal), with this source-level change for exotic object_t::key_types.

5. Nit

Double blank line after the write_msgpack_string template, in both include/nlohmann/detail/output/binary_writer.hpp and the amalgamated single_include/nlohmann/json.hpp. astyle doesn't collapse those, so CI stayed green.

Optional

There's no regression test guarding the allocation count, so this could silently regress later. Given it needs a global operator new override, it's probably not worth adding to the doctest binaries — just noting it.

_{Reviewed with the help of Claude Code.}

[nlohmann]

Check #5328 (comment)

[alexprabhat99]

Hi @nlohmann
I have addressed the review comments. The one failing test seems unrelated to the PR changes.

[alexprabhat99]

Hi @nlohmann
All checks have passed.

[alexprabhat99]

Hi @nlohmann
Gentle ping! All checks have passed.

[nlohmann]

Thanks a lot, and sorry for the delay.

Two issues I would like you to address:

1. A static_assert on std::is_convertible<object_t::key_type, string_t> in the two key loops, so a rejected key type produces one readable line instead of a template wall.
2. One sentence in docs/mkdocs/docs/api/basic_json/object_t.md stating that object_t::key_type must be convertible to string_t. The page already implies it by describing the type as ObjectType<StringType, ...>, but the new tests make it a contract, so let's write it down.

Minor: the key in tests/src/custom_object_key_type.hpp carries a c_str() that no CBOR/MsgPack test uses. That's what UBJSON would need - it calls el.first.size() and .c_str() on the key directly - and in fact this key type still doesn't compile with to_ubjson. Pre-existing and out of scope here, but it means "custom key types are supported" isn't uniformly true yet. Either drop the unused c_str() or leave a comment saying why it's there; the UBJSON gap can be a follow-up issue.

[nlohmann]

Automated review findings — posted via Claude Code on behalf of @nlohmann.

Two issues found with high confidence:

1. The static_assert added to single_include/nlohmann/json.hpp guarding object_t::key_type convertibility isn't mirrored in the modular include/ header it's generated from. Since single_include is produced from include/ via make amalgamate, this diff will fail the check_amalgamation CI job, and the modular-header build silently loses the static_assert.
2. The new custom_object_key_type.hpp test fixture is missing a size() method to match its c_str() shim, making it incompatible with UBJSON's key-writing path (verified by compiling to_ubjson against it) despite the comment claiming broader serializer compatibility.

See inline comments below for exact locations.

[nlohmann]

Please rebase to the latest develop to resolve conflicts.

[nlohmann]

@alexprabhat99 Are you willing to continue working on this?

[nlohmann]

Automated review — posted via Claude Code on behalf of @nlohmann.

Thanks for the rebase! All points from the previous rounds are addressed: the static_asserts are in both include/ and single_include/, object_t.md documents the convertibility requirement, the test header has the SPDX banner, and the template overloads are gone in favor of const string_t& (consistent with the BSON writer). make amalgamate produces no diff, and unit-cbor / unit-msgpack pass locally (C++11 and C++20 for the new test cases, full suites under C++11).

Only nits remain, see inline. One more: please mention the last_child() change in the PR description, since it fixes a develop build break (unit-custom-object-type.cpp fails to compile since #5762 because no_key_compare_map has no rbegin()) that is unrelated to #5318.

[nlohmann]

FYI: I opened #5767 to fix develop.

[nlohmann]

Thanks for the update, @alexprabhat99! The approach is sound: writing keys through write_cbor_string / write_msgpack_string with the object as diagnostics context keeps the error behavior of the old check_utf8 + write_cbor(el.first) pair without the temporary basic_json. The static_asserts are now in both headers, and dropping the rbegin() change was right, since #5767 fixed that on develop separately.

Here is what is left before this can go in.

1. Rebase onto develop, which now has a second key-writing path (blocking)

The branch conflicts with develop in binary_writer.hpp, single_include/nlohmann/json.hpp, and tests/src/unit-msgpack.cpp. Most of that comes from #5781 ("Stop binary writers overflowing the stack on deep values"). It added write_cbor_iterative and write_msgpack_iterative, which take over below detail::recursion_depth_limit() (128 levels). Each has its own copy of the old key handling:

if (error_handler == error_handler_t::strict)
{
    check_utf8(current.object_it->first, *current.value);
}
write_cbor(current.object_it->first);   // still a temporary basic_json

The same pattern is in write_msgpack_iterative with write_msgpack(...). Please convert both to the new helpers during the rebase:

write_cbor_string(current.object_it->first, *current.value);

Without that, every key below level 128 still goes through a temporary basic_json. That is what this PR removes, and the new tests wouldn't notice. It would be good to extend both tests with a value nested deeper than 128 objects, using the custom key type, so the iterative path gets a round trip too.

2. size() and c_str() in the test key type: my earlier advice was wrong

In the last two rounds I said UBJSON reads object keys through el.first.size() and el.first.c_str(). That was incorrect, sorry for the extra churn. On this PR's base and on develop, write_ubjson passes keys through sanitize_utf8_for_write(el.first, j, storage), like the other writers. The only place the library touches a key's own members is data() in exceptions.hpp, for JSON_DIAGNOSTICS. Please remove size(), c_str(), and their comment, and keep data() with its comment (see inline). Please also don't reuse this fixture for UBJSON in this PR; that path has a separate problem with non-string_t keys that I'll handle elsewhere.

3. CI

The failures on the current run aren't caused by this PR. ci_clang_tidy, for example, fails with no member named 'rbegin' in no_key_compare_map in unit-custom-object-type.cpp, the develop breakage fixed by #5767 and #5779. CI on develop is green again, so a rebase should give a clean run. DCO is fine.

Will you continue?

Are you willing and able to continue working on this? Rebasing is the main remaining work. If you don't have time soon, you could tick "Allow edits by maintainers" on this PR (it is currently off) so we can do the rebase for you, keeping your authorship and sign-off. Either way is fine, just let us know.

_{This comment was written by Claude Code on behalf of @nlohmann.}

[nlohmann]

Looks good to me.

[nlohmann]

Thanks a lot! FYI I found an unrelated bug so there will be a follow-up PR.