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.

[gregmarr]

Looks like you added this manually and only to the single_include file.

[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]

The comment says this is "kept for compatibility with serializers that access object keys as C strings," but there's no matching size(). UBJSON's write_ubjson accesses object keys via el.first.size() and el.first.c_str() directly (not through the string_t conversion operator CBOR/MsgPack/BSON use), so custom_object_key_test::json::to_ubjson(...) fails to compile with "no member named 'size' in 'custom_object_key_test::key'" if this shared fixture is ever reused for a UBJSON test. Not exercised by this PR (CBOR/MsgPack only), so it's latent for now.

[nlohmann]

Posted via Claude Code on behalf of @nlohmann.

Still applies to the reworded comment: UBJSON needs both size() and c_str() on the key, so this type still does not work with to_ubjson, and "serialization paths that access object keys through c_str()" suggests otherwise. Since no test here needs it, I would simply drop c_str(); the UBJSON gap can be a separate follow-up.

[nlohmann]

Please rebase to the latest develop to resolve conflicts.

[nlohmann]

@alexprabhat99 Are you willing to continue working on this?

[alexprabhat99]

@nlohmann Apologies. I will resolve the conflicts as soon as I get time.

[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]

This is right — develop currently fails to compile unit-custom-object-type.cpp since #5762 because no_key_compare_map has no rbegin(), so thanks for fixing it along the way.

Nit: the comment in pop_last_child() below still says std::prev(end()) is used "rather than rbegin() (see last_child() above)", which is now circular since last_child() no longer uses rbegin() either. Could you reword one of the two, e.g. say in one place that std::prev(end()) is used in both because a custom object_t need not provide rbegin() and erase() needs a forward iterator?

[nlohmann]

FYI: I opened #5767 to fix develop.