Skip to content

Fix destroy() for ObjectTypes without reverse iteration - #5767

Merged
nlohmann merged 1 commit into
developfrom
fix/destroy-forward-object-iterators
Oct 6, 2026
Merged

nlohmann merged 1 commit into
developfrom
fix/destroy-forward-object-iterators

Conversation

@nlohmann

@nlohmann nlohmann commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Since #5762 (commit 0a36586), develop no longer compiles tests/src/unit-custom-object-type.cpp:

include/nlohmann/json.hpp:648: error: no member named 'rbegin' in '(anonymous namespace)::no_key_compare_map<...>'

The non-recursive, allocation-free destroy walk picked an object's last child via object_t::rbegin() (in last_child()) and removed it with erase(std::prev(end())) (in pop_last_child()). Neither is part of the ObjectType requirements:

  • no_key_compare_map (in that test) has no rbegin();
  • hash maps such as std::unordered_map (through the documented adapter) only have forward iterators, so std::prev(end()) does not compile either. iter_impl already states that object_t::iterator may be forward-only as long as reverse iteration and operator-- are not used.

Fix

The walk can take a container's children in any order, as long as it finds the same child again while that container is not modified, which holds because a parent is never touched while the walk is below it. The helpers are renamed to walk_child() / pop_walk_child(). An object's child is chosen by tag dispatch on its iterator category:

  • bidirectional (or better) iterators (std::map, ordered_map, boost::container::flat_map, ...): last child, erase(std::prev(end())). This is what the code did before, now without rbegin(), so vector-based maps still remove each child in O(1) and destruction stays linear.
  • forward-only iterators (hash maps): first child, erase(begin()). For node-based hash maps such as std::unordered_map, begin() and erase are O(1).

Arrays are unchanged (back() / pop_back()). Nothing allocates, and the comments in destroy_container() now describe the "walk child" instead of the "last child".

One caveat for forward-only maps: open-addressing hash maps whose begin() scans for the first occupied slot (e.g. absl::flat_hash_map) can make repeated erase(begin()) superlinear for very wide objects. On develop such maps do not compile at all. Compared with 3.12.0, whose destroy() moved children onto a heap-allocated stack in linear time, destroying very wide objects of such maps can be slower. Making the walk independent of begin()'s cost would need an iterator stored per level, which the allocation-free design has no room for.

Tests

  • unit-custom-object-type.cpp compiles again (no_key_compare_map).
  • New forward_only_map ObjectType whose iterator is std::forward_iterator_tag (no rbegin(), no operator--). It is destroyed with mixed nested objects/arrays, through erase() and assignment, and 100000 levels deep. Against develop's header it fails to compile (rbegin and the std::prev static assertion), and it passes with this change (C++11/C++20, and with ASan/UBSan + JSON_DIAGNOSTICS).
  • Full CMake suite (-DJSON_BuildTests=ON, json_test_data) passes locally: 100% tests passed, 0 tests failed out of 139 (Debug, AppleClang, macOS).

Public API

No breaking changes. The changed helpers are private members of basic_json::json_value. For ObjectTypes with bidirectional iterators the behavior is identical. ObjectTypes with forward-only iterators or without rbegin() compile again, as they did before #5762.


This PR was written by Claude Code.

🤖 Generated with Claude Code

The non-recursive destroy walk from #5762 picked an object's last child
via object_t::rbegin() and std::prev(end()). Neither is available for
every ObjectType: no_key_compare_map in unit-custom-object-type.cpp has
no rbegin(), so develop no longer compiles that test, and hash maps such
as std::unordered_map only have forward iterators.

The walk can take an object's children in any order, as long as it
finds the same child again while the object is not modified in between.
So objects with bidirectional iterators keep using their last child
(O(1) to remove from vector-based maps like ordered_map), and objects
with forward-only iterators use begin() instead. No reverse iteration
or rbegin() is needed any more, and the walk stays allocation-free.

Adds a forward-only ObjectType to the tests, destroyed both with mixed
nesting and 100000 levels deep.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Oct 6, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 6, 2026
@nlohmann
nlohmann merged commit 6d4543e into develop Oct 6, 2026
144 of 169 checks passed
@nlohmann
nlohmann deleted the fix/destroy-forward-object-iterators branch October 6, 2026 20:29
@nlohmann nlohmann mentioned this pull request Oct 7, 2026
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants