From 37316992ede23e4e5b41f68dbb457a2fa29d683a Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Tue, 6 Oct 2026 09:21:08 +0200 Subject: [PATCH] Fix destroy() for ObjectTypes without reverse iteration 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 --- include/nlohmann/json.hpp | 77 ++++++--- single_include/nlohmann/json.hpp | 77 ++++++--- tests/src/unit-custom-object-type.cpp | 237 ++++++++++++++++++++++++++ 3 files changed, 343 insertions(+), 48 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index aa8b1a6c6..f698163fe 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -638,21 +638,49 @@ private: } } - static basic_json& last_child(basic_json& v) + // The walk in destroy_container() may take the children of a + // container in any order, as long as it picks the same child again + // while that container is not modified in between. Arrays and + // objects with bidirectional iterators (std::map, ordered_map, ...) + // use their last child, which a vector-based container can remove + // in O(1). ObjectType only needs forward iterators, though (e.g. + // std::unordered_map), so other objects use their first child. + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::bidirectional_iterator_tag /*unused*/) + { + return std::prev(o.end()); + } + + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::forward_iterator_tag /*unused*/) + { + return o.begin(); + } + + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o) + { + JSON_ASSERT(!o.empty()); + return walk_child_it(o, typename std::iterator_traits::iterator_category()); + } + + // the child of a non-empty array/object v that the walk in + // destroy_container() continues with (see walk_child_it() above) + static basic_json& walk_child(basic_json& v) { if (v.m_data.m_type == value_t::array) { return v.m_data.m_value.array->back(); } JSON_ASSERT(v.m_data.m_type == value_t::object); - return v.m_data.m_value.object->rbegin()->second; + return walk_child_it(*v.m_data.m_value.object)->second; } - // removes the last child of a non-empty array/object v; this never + // removes walk_child(v) from a non-empty array/object v; this never // allocates, and since it is only ever called when that child is a // scalar or an already-empty array/object, destroying it never // recurses more than one level deep (see destroy() below) - static void pop_last_child(basic_json& v) + static void pop_walk_child(basic_json& v) { if (v.m_data.m_type == value_t::array) { @@ -661,9 +689,7 @@ private: else { JSON_ASSERT(v.m_data.m_type == value_t::object); - // erase() needs a forward iterator, so std::prev(end()) is - // used here rather than rbegin() (see last_child() above) - v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end())); + v.m_data.m_value.object->erase(walk_child_it(*v.m_data.m_value.object)); } } @@ -734,14 +760,17 @@ public: // bad_alloc, which would escape this noexcept destructor and // terminate the program (#5135). // - // Instead, walk down the "last child" chain, reversing links - // as we go: cur is the container currently being emptied, - // and prev is its parent (value_t::null when there is none). - // Each parent's last child slot doubles as storage for that - // parent's own parent link while we are below it, so no - // extra memory is needed. We only ever remove a child once - // it is a scalar or an empty array/object, which neither - // allocates nor recurses more than one level deep. + // Instead, walk down a chain of children (always the one + // walk_child() picks), reversing links as we go: cur is the + // container currently being emptied, and prev is its parent + // (value_t::null when there is none). Each parent's + // walk_child() slot doubles as storage for that parent's own + // parent link while we are below it, so no extra memory is + // needed; the parent is not modified meanwhile, so + // walk_child() finds that same slot again on the way up. We + // only ever remove a child once it is a scalar or an empty + // array/object, which neither allocates nor recurses more + // than one level deep. // // This json_value is not itself a basic_json, so the // top-level container is first moved into a local stand-in @@ -767,11 +796,11 @@ public: } // ascend: detach the grandparent link from prev's - // last slot, drop that (now null) slot, free cur + // walk_child() slot, drop that (now null) slot, free cur // (it is empty), then move up one level basic_json gp; - take(gp, last_child(prev)); - pop_last_child(prev); + take(gp, walk_child(prev)); + pop_walk_child(prev); free_container(cur); @@ -780,21 +809,21 @@ public: continue; } - basic_json& cur_last_ref = last_child(cur); + basic_json& cur_child_ref = walk_child(cur); - if (has_no_children(cur_last_ref)) + if (has_no_children(cur_child_ref)) { // scalar, or already-empty array/object - pop_last_child(cur); + pop_walk_child(cur); continue; } - // descend into the non-empty last child, reversing the + // descend into the non-empty child, reversing the // link: its slot takes over prev, and the child becomes // the new cur basic_json tmp; - take(tmp, cur_last_ref); - take(cur_last_ref, prev); + take(tmp, cur_child_ref); + take(cur_child_ref, prev); take(prev, cur); take(cur, tmp); } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 00e345dd3..b6e5ca7c5 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -27877,21 +27877,49 @@ private: } } - static basic_json& last_child(basic_json& v) + // The walk in destroy_container() may take the children of a + // container in any order, as long as it picks the same child again + // while that container is not modified in between. Arrays and + // objects with bidirectional iterators (std::map, ordered_map, ...) + // use their last child, which a vector-based container can remove + // in O(1). ObjectType only needs forward iterators, though (e.g. + // std::unordered_map), so other objects use their first child. + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::bidirectional_iterator_tag /*unused*/) + { + return std::prev(o.end()); + } + + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::forward_iterator_tag /*unused*/) + { + return o.begin(); + } + + template + static typename ObjectType_::iterator walk_child_it(ObjectType_& o) + { + JSON_ASSERT(!o.empty()); + return walk_child_it(o, typename std::iterator_traits::iterator_category()); + } + + // the child of a non-empty array/object v that the walk in + // destroy_container() continues with (see walk_child_it() above) + static basic_json& walk_child(basic_json& v) { if (v.m_data.m_type == value_t::array) { return v.m_data.m_value.array->back(); } JSON_ASSERT(v.m_data.m_type == value_t::object); - return v.m_data.m_value.object->rbegin()->second; + return walk_child_it(*v.m_data.m_value.object)->second; } - // removes the last child of a non-empty array/object v; this never + // removes walk_child(v) from a non-empty array/object v; this never // allocates, and since it is only ever called when that child is a // scalar or an already-empty array/object, destroying it never // recurses more than one level deep (see destroy() below) - static void pop_last_child(basic_json& v) + static void pop_walk_child(basic_json& v) { if (v.m_data.m_type == value_t::array) { @@ -27900,9 +27928,7 @@ private: else { JSON_ASSERT(v.m_data.m_type == value_t::object); - // erase() needs a forward iterator, so std::prev(end()) is - // used here rather than rbegin() (see last_child() above) - v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end())); + v.m_data.m_value.object->erase(walk_child_it(*v.m_data.m_value.object)); } } @@ -27973,14 +27999,17 @@ public: // bad_alloc, which would escape this noexcept destructor and // terminate the program (#5135). // - // Instead, walk down the "last child" chain, reversing links - // as we go: cur is the container currently being emptied, - // and prev is its parent (value_t::null when there is none). - // Each parent's last child slot doubles as storage for that - // parent's own parent link while we are below it, so no - // extra memory is needed. We only ever remove a child once - // it is a scalar or an empty array/object, which neither - // allocates nor recurses more than one level deep. + // Instead, walk down a chain of children (always the one + // walk_child() picks), reversing links as we go: cur is the + // container currently being emptied, and prev is its parent + // (value_t::null when there is none). Each parent's + // walk_child() slot doubles as storage for that parent's own + // parent link while we are below it, so no extra memory is + // needed; the parent is not modified meanwhile, so + // walk_child() finds that same slot again on the way up. We + // only ever remove a child once it is a scalar or an empty + // array/object, which neither allocates nor recurses more + // than one level deep. // // This json_value is not itself a basic_json, so the // top-level container is first moved into a local stand-in @@ -28006,11 +28035,11 @@ public: } // ascend: detach the grandparent link from prev's - // last slot, drop that (now null) slot, free cur + // walk_child() slot, drop that (now null) slot, free cur // (it is empty), then move up one level basic_json gp; - take(gp, last_child(prev)); - pop_last_child(prev); + take(gp, walk_child(prev)); + pop_walk_child(prev); free_container(cur); @@ -28019,21 +28048,21 @@ public: continue; } - basic_json& cur_last_ref = last_child(cur); + basic_json& cur_child_ref = walk_child(cur); - if (has_no_children(cur_last_ref)) + if (has_no_children(cur_child_ref)) { // scalar, or already-empty array/object - pop_last_child(cur); + pop_walk_child(cur); continue; } - // descend into the non-empty last child, reversing the + // descend into the non-empty child, reversing the // link: its slot takes over prev, and the child becomes // the new cur basic_json tmp; - take(tmp, cur_last_ref); - take(cur_last_ref, prev); + take(tmp, cur_child_ref); + take(cur_child_ref, prev); take(prev, cur); take(cur, tmp); } diff --git a/tests/src/unit-custom-object-type.cpp b/tests/src/unit-custom-object-type.cpp index a579e0b2d..0eaaf2d3c 100644 --- a/tests/src/unit-custom-object-type.cpp +++ b/tests/src/unit-custom-object-type.cpp @@ -10,7 +10,9 @@ #include +#include #include +#include #include #include #include @@ -196,6 +198,198 @@ struct void_erase_map : std::map using void_erase_json = nlohmann::basic_json; +// wraps an iterator, but only offers the LegacyForwardIterator operations, +// like the iterators of std::unordered_map and other hash maps +template +class forward_only_iterator +{ + BaseIterator m_it{}; + + public: + using iterator_category = std::forward_iterator_tag; + using value_type = typename std::iterator_traits::value_type; + using difference_type = typename std::iterator_traits::difference_type; + using pointer = typename std::iterator_traits::pointer; + using reference = typename std::iterator_traits::reference; + + forward_only_iterator() = default; + explicit forward_only_iterator(BaseIterator it) : m_it(it) {} + + BaseIterator base() const + { + return m_it; + } + + reference operator*() const + { + return *m_it; + } + pointer operator->() const + { + return &*m_it; + } + forward_only_iterator& operator++() + { + ++m_it; + return *this; + } + forward_only_iterator operator++(int) + { + auto result = *this; + ++m_it; + return result; + } + + friend bool operator==(const forward_only_iterator& lhs, const forward_only_iterator& rhs) + { + return lhs.m_it == rhs.m_it; + } + friend bool operator!=(const forward_only_iterator& lhs, const forward_only_iterator& rhs) + { + return lhs.m_it != rhs.m_it; + } +}; + +// An ObjectType whose iterators are forward-only, as those of hash maps are; +// it has no rbegin() and its iterators no operator--. A hash map is not used +// directly for the same reason as in no_key_compare_map above. +template +class forward_only_map +{ + using map_t = std::map; + map_t data; + + public: + using key_type = typename map_t::key_type; + using mapped_type = typename map_t::mapped_type; + using value_type = typename map_t::value_type; + using size_type = typename map_t::size_type; + using allocator_type = typename map_t::allocator_type; + using iterator = forward_only_iterator; + using const_iterator = forward_only_iterator; + + forward_only_map() noexcept(std::is_nothrow_default_constructible::value) : data() {} + + template + forward_only_map(InputIt first, InputIt last) : data(first, last) {} + + iterator begin() noexcept + { + return iterator(data.begin()); + } + iterator end() noexcept + { + return iterator(data.end()); + } + const_iterator begin() const noexcept + { + return const_iterator(data.begin()); + } + const_iterator end() const noexcept + { + return const_iterator(data.end()); + } + const_iterator cbegin() const noexcept + { + return const_iterator(data.cbegin()); + } + const_iterator cend() const noexcept + { + return const_iterator(data.cend()); + } + + bool empty() const noexcept + { + return data.empty(); + } + size_type size() const noexcept + { + return data.size(); + } + size_type max_size() const noexcept + { + return data.max_size(); + } + void clear() noexcept + { + data.clear(); + } + + iterator find(const key_type& key) + { + return iterator(data.find(key)); + } + const_iterator find(const key_type& key) const + { + return const_iterator(data.find(key)); + } + size_type count(const key_type& key) const + { + return data.count(key); + } + + std::pair emplace(const key_type& key, const mapped_type& value) + { + const auto result = data.emplace(key, value); + return {iterator(result.first), result.second}; + } + + std::pair insert(const value_type& value) + { + const auto result = data.insert(value); + return {iterator(result.first), result.second}; + } + + template + void insert(InputIt first, InputIt last) + { + data.insert(first, last); + } + + mapped_type& operator[](const key_type& key) + { + return data[key]; + } + + mapped_type& at(const key_type& key) + { + return data.at(key); + } + const mapped_type& at(const key_type& key) const + { + return data.at(key); + } + + iterator erase(iterator pos) + { + return iterator(data.erase(pos.base())); + } + iterator erase(iterator first, iterator last) + { + return iterator(data.erase(first.base(), last.base())); + } + size_type erase(const key_type& key) + { + return data.erase(key); + } + + void swap(forward_only_map& other) noexcept(noexcept(data.swap(other.data))) + { + data.swap(other.data); + } + + friend bool operator==(const forward_only_map& lhs, const forward_only_map& rhs) + { + return lhs.data == rhs.data; + } + friend bool operator<(const forward_only_map& lhs, const forward_only_map& rhs) + { + return lhs.data < rhs.data; + } +}; + +using forward_only_json = nlohmann::basic_json; + } // namespace TEST_CASE("object type whose erase() returns void") @@ -322,3 +516,46 @@ TEST_CASE("object type without key_compare") } } + +TEST_CASE("object type with forward-only iterators") +{ + CHECK(std::is_same::iterator_category, + std::forward_iterator_tag>::value); + + SECTION("destroying nested objects and arrays") + { + forward_only_json j; + j["a"] = 1; + j["b"]["c"] = "x"; + j["b"]["d"] = forward_only_json::array(); + j["b"]["d"].push_back(forward_only_json::object()); + j["b"]["d"].push_back(true); + j["b"]["e"]["f"]["g"] = nullptr; + j["h"] = forward_only_json::object(); + j["i"]["j"] = 2; + + CHECK(j.size() == 4); + CHECK(j["b"].size() == 3); + CHECK(j["b"]["d"].size() == 2); + CHECK(j["b"]["e"]["f"]["g"].is_null()); + + CHECK(j.erase("b") == 1); + CHECK(j.size() == 3); + j = 42; + CHECK(j == 42); + } + + SECTION("destroying a deeply nested object") + { + constexpr std::size_t depth = 100000; + forward_only_json j; + forward_only_json* cur = &j; + for (std::size_t i = 0; i < depth; ++i) + { + (*cur)["s"] = i; + cur = &(*cur)["o"]; + } + CHECK(j["o"]["o"]["s"] == 2); + // destroyed at the end of scope without recursing per level + } +}