From daad972cdbd1e6805e8a873d55795e2003a6d423 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Mon, 5 Oct 2026 08:56:46 +0200 Subject: [PATCH] Refactor destroy() for readability and add edge-case tests Apply review feedback from Greg Marr on the json_value::destroy() non-recursive, allocation-free destruction walk (#5135): - last_child() now uses object->rbegin()->second instead of std::prev(object->end())->second; pop_last_child() keeps std::prev(end()) since erase() needs a forward iterator. - is_empty_container() becomes has_no_children(), a switch that returns true for every non-container type as well as empty array/object, simplifying the "scalar or already-empty child" check at the call site. The local variable `last` is renamed to `cur_last_ref` for clarity. - free_container() asserts the array/object is already empty before freeing it, and the object branches assert the expected type. - destroy(value_t t) is now a thin dispatcher to destroy_string(), destroy_binary(), and destroy_container(t), each handling its own "not initialized" check and sharing the simple cases first in the switch. - destroy_container() moves the top-level container into the local stand-in via a plain swap of the json_value union, instead of a manual copy plus clearing array/object by hand. - The "cur has no children and there is no parent" case now frees cur and returns immediately, so the main loop is a plain while (true) with no trailing code after it. Also adds edge-case tests for both json and ordered_json (mixes of empty/non-empty arrays and objects, container children in first/last position, single-element chains, top-level empty containers, and destruction via erase()/assignment), plus a mixed-tree case in the "destructor performs no allocation" test. Signed-off-by: Niels Lohmann --- include/nlohmann/json.hpp | 215 +++++++++++++++++-------------- single_include/nlohmann/json.hpp | 215 +++++++++++++++++-------------- tests/src/unit-allocator.cpp | 16 +++ tests/src/unit-regression2.cpp | 111 ++++++++++++++++ 4 files changed, 369 insertions(+), 188 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index e21a07bfe..2af32ebaf 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -623,18 +623,28 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec src.m_data.m_type = value_t::null; } - static bool is_empty_container(const basic_json& v) noexcept + // true if v is not an array/object, or is an already-empty one + static bool has_no_children(const basic_json& v) noexcept { - return v.m_data.m_type == value_t::array - ? v.m_data.m_value.array->empty() - : v.m_data.m_value.object->empty(); + switch (v.m_data.m_type) + { + case value_t::array: + return v.m_data.m_value.array->empty(); + case value_t::object: + return v.m_data.m_value.object->empty(); + default: + return true; + } } static basic_json& last_child(basic_json& v) { - return v.m_data.m_type == value_t::array - ? v.m_data.m_value.array->back() - : std::prev(v.m_data.m_value.object->end())->second; + 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; } // removes the last child of a non-empty array/object v; this never @@ -649,6 +659,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } 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())); } } @@ -661,12 +674,15 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { if (v.m_data.m_type == value_t::array) { + JSON_ASSERT(v.m_data.m_value.array->empty()); AllocatorType alloc; std::allocator_traits::destroy(alloc, v.m_data.m_value.array); std::allocator_traits::deallocate(alloc, v.m_data.m_value.array, 1); } else { + JSON_ASSERT(v.m_data.m_type == value_t::object); + JSON_ASSERT(v.m_data.m_value.object->empty()); AllocatorType alloc; std::allocator_traits::destroy(alloc, v.m_data.m_value.object); std::allocator_traits::deallocate(alloc, v.m_data.m_value.object, 1); @@ -674,117 +690,130 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec v.m_data.m_type = value_t::null; // avoid a double free if v is later destructed } - void destroy(value_t t) + void destroy_string() noexcept + { + if (string == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, string); + std::allocator_traits::deallocate(alloc, string, 1); + } + + void destroy_binary() noexcept + { + if (binary == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, binary); + std::allocator_traits::deallocate(alloc, binary, 1); + } + + // t must be value_t::array or value_t::object + void destroy_container(value_t t) noexcept { if ( (t == value_t::object && object == nullptr) || - (t == value_t::array && array == nullptr) || - (t == value_t::string && string == nullptr) || - (t == value_t::binary && binary == nullptr) + (t == value_t::array && array == nullptr) ) { // not initialized (e.g., due to exception in the ctor) return; } - if (t == value_t::array || t == value_t::object) + + // Destroy the tree without recursing per nesting level and + // without any heap allocation: a heap-allocated flattening + // stack (the previous implementation) can itself throw + // 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. + // + // This json_value is not itself a basic_json, so the + // top-level container is first moved into a local stand-in + // ("cur"); a default-constructed basic_json has a null + // pointer in its m_value (see data::m_value's initializer), + // so swapping it with *this leaves this union's own pointer + // null, and it is never looked at or freed a second time. + basic_json cur; + cur.m_data.m_type = t; + using std::swap; + swap(cur.m_data.m_value, *this); + + basic_json prev; // value_t::null: no parent + + while (true) { - // Destroy the tree without recursing per nesting level and - // without any heap allocation: a heap-allocated flattening - // stack (the previous implementation) can itself throw - // 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. - // - // This json_value is not itself a basic_json, so the - // top-level container is first moved into a local stand-in - // ("cur"); this union's own pointer is cleared so it is - // never looked at or freed a second time. - basic_json cur; - cur.m_data.m_type = t; - cur.m_data.m_value = *this; - if (t == value_t::array) + if (has_no_children(cur)) { - array = nullptr; - } - else - { - object = nullptr; - } - - basic_json prev; // value_t::null: no parent - - while (true) - { - if (is_empty_container(cur)) + if (prev.m_data.m_type == value_t::null) { - if (prev.m_data.m_type == value_t::null) - { - break; // back at the top with nothing left to do - } - - // ascend: detach the grandparent link from prev's - // last 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); - free_container(cur); - - take(cur, prev); - take(prev, gp); - continue; + return; // back at the top with nothing left to do } - basic_json& last = last_child(cur); - const bool last_is_container = last.m_data.m_type == value_t::array || last.m_data.m_type == value_t::object; + // ascend: detach the grandparent link from prev's + // last 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); - if (!last_is_container || is_empty_container(last)) - { - // scalar, or already-empty array/object - pop_last_child(cur); - continue; - } + free_container(cur); - // descend into the non-empty last child, reversing the - // link: its slot takes over prev, and the child becomes - // the new cur - basic_json tmp; - take(tmp, last); - take(last, prev); - take(prev, cur); - take(cur, tmp); + take(cur, prev); + take(prev, gp); + continue; } - free_container(cur); - return; - } + basic_json& cur_last_ref = last_child(cur); + if (has_no_children(cur_last_ref)) + { + // scalar, or already-empty array/object + pop_last_child(cur); + continue; + } + + // descend into the non-empty last 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(prev, cur); + take(cur, tmp); + } + } + + void destroy(value_t t) + { switch (t) { case value_t::string: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, string); - std::allocator_traits::deallocate(alloc, string, 1); + destroy_string(); break; - } case value_t::binary: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, binary); - std::allocator_traits::deallocate(alloc, binary, 1); + destroy_binary(); + break; + + case value_t::object: + case value_t::array: + destroy_container(t); break; - } case value_t::null: case value_t::boolean: @@ -793,9 +822,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::number_float: case value_t::discarded: default: - { break; - } } } }; diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index a0a040424..999f5e577 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -27745,18 +27745,28 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec src.m_data.m_type = value_t::null; } - static bool is_empty_container(const basic_json& v) noexcept + // true if v is not an array/object, or is an already-empty one + static bool has_no_children(const basic_json& v) noexcept { - return v.m_data.m_type == value_t::array - ? v.m_data.m_value.array->empty() - : v.m_data.m_value.object->empty(); + switch (v.m_data.m_type) + { + case value_t::array: + return v.m_data.m_value.array->empty(); + case value_t::object: + return v.m_data.m_value.object->empty(); + default: + return true; + } } static basic_json& last_child(basic_json& v) { - return v.m_data.m_type == value_t::array - ? v.m_data.m_value.array->back() - : std::prev(v.m_data.m_value.object->end())->second; + 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; } // removes the last child of a non-empty array/object v; this never @@ -27771,6 +27781,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } 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())); } } @@ -27783,12 +27796,15 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { if (v.m_data.m_type == value_t::array) { + JSON_ASSERT(v.m_data.m_value.array->empty()); AllocatorType alloc; std::allocator_traits::destroy(alloc, v.m_data.m_value.array); std::allocator_traits::deallocate(alloc, v.m_data.m_value.array, 1); } else { + JSON_ASSERT(v.m_data.m_type == value_t::object); + JSON_ASSERT(v.m_data.m_value.object->empty()); AllocatorType alloc; std::allocator_traits::destroy(alloc, v.m_data.m_value.object); std::allocator_traits::deallocate(alloc, v.m_data.m_value.object, 1); @@ -27796,117 +27812,130 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec v.m_data.m_type = value_t::null; // avoid a double free if v is later destructed } - void destroy(value_t t) + void destroy_string() noexcept + { + if (string == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, string); + std::allocator_traits::deallocate(alloc, string, 1); + } + + void destroy_binary() noexcept + { + if (binary == nullptr) + { + // not initialized (e.g., due to exception in the ctor) + return; + } + AllocatorType alloc; + std::allocator_traits::destroy(alloc, binary); + std::allocator_traits::deallocate(alloc, binary, 1); + } + + // t must be value_t::array or value_t::object + void destroy_container(value_t t) noexcept { if ( (t == value_t::object && object == nullptr) || - (t == value_t::array && array == nullptr) || - (t == value_t::string && string == nullptr) || - (t == value_t::binary && binary == nullptr) + (t == value_t::array && array == nullptr) ) { // not initialized (e.g., due to exception in the ctor) return; } - if (t == value_t::array || t == value_t::object) + + // Destroy the tree without recursing per nesting level and + // without any heap allocation: a heap-allocated flattening + // stack (the previous implementation) can itself throw + // 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. + // + // This json_value is not itself a basic_json, so the + // top-level container is first moved into a local stand-in + // ("cur"); a default-constructed basic_json has a null + // pointer in its m_value (see data::m_value's initializer), + // so swapping it with *this leaves this union's own pointer + // null, and it is never looked at or freed a second time. + basic_json cur; + cur.m_data.m_type = t; + using std::swap; + swap(cur.m_data.m_value, *this); + + basic_json prev; // value_t::null: no parent + + while (true) { - // Destroy the tree without recursing per nesting level and - // without any heap allocation: a heap-allocated flattening - // stack (the previous implementation) can itself throw - // 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. - // - // This json_value is not itself a basic_json, so the - // top-level container is first moved into a local stand-in - // ("cur"); this union's own pointer is cleared so it is - // never looked at or freed a second time. - basic_json cur; - cur.m_data.m_type = t; - cur.m_data.m_value = *this; - if (t == value_t::array) + if (has_no_children(cur)) { - array = nullptr; - } - else - { - object = nullptr; - } - - basic_json prev; // value_t::null: no parent - - while (true) - { - if (is_empty_container(cur)) + if (prev.m_data.m_type == value_t::null) { - if (prev.m_data.m_type == value_t::null) - { - break; // back at the top with nothing left to do - } - - // ascend: detach the grandparent link from prev's - // last 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); - free_container(cur); - - take(cur, prev); - take(prev, gp); - continue; + return; // back at the top with nothing left to do } - basic_json& last = last_child(cur); - const bool last_is_container = last.m_data.m_type == value_t::array || last.m_data.m_type == value_t::object; + // ascend: detach the grandparent link from prev's + // last 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); - if (!last_is_container || is_empty_container(last)) - { - // scalar, or already-empty array/object - pop_last_child(cur); - continue; - } + free_container(cur); - // descend into the non-empty last child, reversing the - // link: its slot takes over prev, and the child becomes - // the new cur - basic_json tmp; - take(tmp, last); - take(last, prev); - take(prev, cur); - take(cur, tmp); + take(cur, prev); + take(prev, gp); + continue; } - free_container(cur); - return; - } + basic_json& cur_last_ref = last_child(cur); + if (has_no_children(cur_last_ref)) + { + // scalar, or already-empty array/object + pop_last_child(cur); + continue; + } + + // descend into the non-empty last 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(prev, cur); + take(cur, tmp); + } + } + + void destroy(value_t t) + { switch (t) { case value_t::string: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, string); - std::allocator_traits::deallocate(alloc, string, 1); + destroy_string(); break; - } case value_t::binary: - { - AllocatorType alloc; - std::allocator_traits::destroy(alloc, binary); - std::allocator_traits::deallocate(alloc, binary, 1); + destroy_binary(); + break; + + case value_t::object: + case value_t::array: + destroy_container(t); break; - } case value_t::null: case value_t::boolean: @@ -27915,9 +27944,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::number_float: case value_t::discarded: default: - { break; - } } } }; diff --git a/tests/src/unit-allocator.cpp b/tests/src/unit-allocator.cpp index eeefd4acd..b65615c03 100644 --- a/tests/src/unit-allocator.cpp +++ b/tests/src/unit-allocator.cpp @@ -670,4 +670,20 @@ TEST_CASE("destructor performs no allocation, only deallocation") CHECK(counting_allocator_allocations == allocations_before); CHECK(counting_allocator_deallocations > deallocations_before); } + + SECTION("mixed tree of empty/non-empty arrays and objects") + { + auto* j = new counting_json( // NOLINT(cppcoreguidelines-owning-memory) + { + {"empty_obj", counting_json::object()}, + {"empty_arr", counting_json::array()}, + {"nested", {{"a", counting_json::array({1, 2, counting_json::object()})}, {"b", 3}}}, + {"tail", counting_json::array({counting_json::array({1}), 2, counting_json::array({3})})} + }); + const auto allocations_before = counting_allocator_allocations; + const auto deallocations_before = counting_allocator_deallocations; + delete j; // NOLINT(cppcoreguidelines-owning-memory) + CHECK(counting_allocator_allocations == allocations_before); + CHECK(counting_allocator_deallocations > deallocations_before); + } } diff --git a/tests/src/unit-regression2.cpp b/tests/src/unit-regression2.cpp index 2bb6ba9f7..637029d45 100644 --- a/tests/src/unit-regression2.cpp +++ b/tests/src/unit-regression2.cpp @@ -1116,4 +1116,115 @@ TEST_CASE("regression test #5135 - destructor never allocates, even under memory } #endif +namespace +{ +// a single-element chain of `depth` arrays, built iteratively (never +// recursing: each wrap only moves the previous, already-built value) +template +BasicJsonType make_single_chain(std::size_t depth) +{ + BasicJsonType v = 1; + for (std::size_t i = 0; i < depth; ++i) + { + BasicJsonType wrapper = BasicJsonType::array(); + wrapper.push_back(std::move(v)); + v = std::move(wrapper); + } + return v; +} + +// copies value first, to make sure nothing was corrupted by building it, +// then lets both the copy and the original destruct via normal scope exit +template +void check_destroy_edge_case(const BasicJsonType& value) +{ + const BasicJsonType copy = value; + CHECK(copy == value); +} +} // namespace + +TEST_CASE_TEMPLATE("regression test #5135 - destroy() edge cases", BasicJsonType, json, ordered_json) +{ + using binary_t = typename BasicJsonType::binary_t; + + SECTION("mix of empty objects, empty arrays, non-empty containers, and scalars") + { + BasicJsonType root = BasicJsonType::array(); + root.push_back(BasicJsonType::object()); + root.push_back(BasicJsonType::array()); + root.push_back(BasicJsonType::object({{"k", 1}})); + root.push_back(BasicJsonType::array({1, 2, 3})); + root.push_back(nullptr); + root.push_back(true); + root.push_back(42); + root.push_back(3.14); + root.push_back("a string"); + root.push_back(BasicJsonType(binary_t({1, 2, 3}))); + check_destroy_edge_case(root); + } + + SECTION("container child in first position only") + { + BasicJsonType root = BasicJsonType::array({BasicJsonType::array({1, 2}), 3, 4, 5}); + check_destroy_edge_case(root); + } + + SECTION("container child in last position only") + { + BasicJsonType root = BasicJsonType::array({1, 2, 3, BasicJsonType::array({4, 5})}); + check_destroy_edge_case(root); + } + + SECTION("container children in first and last position") + { + BasicJsonType root = BasicJsonType::array({BasicJsonType::array({1}), 2, 3, BasicJsonType::array({4})}); + check_destroy_edge_case(root); + } + + SECTION("single-element chain, 1000 levels deep") + { + BasicJsonType root = make_single_chain(1000); + check_destroy_edge_case(root); + } + + SECTION("top-level empty array") + { + BasicJsonType root = BasicJsonType::array(); + check_destroy_edge_case(root); + } + + SECTION("top-level empty object") + { + BasicJsonType root = BasicJsonType::object(); + check_destroy_edge_case(root); + } + + SECTION("object whose last child is a non-empty array whose last child is an empty object") + { + BasicJsonType inner_array = BasicJsonType::array({1, 2, BasicJsonType::object()}); + BasicJsonType root = BasicJsonType::object({{"a", 1}, {"b", inner_array}}); + check_destroy_edge_case(root); + } + + SECTION("destruction via erase() on a deeply nested child") + { + BasicJsonType root = BasicJsonType::array(); + root.push_back(make_single_chain(500)); + root.push_back(BasicJsonType::object({{"k", BasicJsonType::array({1, 2, 3})}})); + // erase() must destroy the removed subtree without recursing or + // allocating beyond what erase() itself needs + root.erase(0); + CAPTURE(root.size()) + CHECK(root.size() == 1); + } + + SECTION("destruction via assignment on a deep tree") + { + BasicJsonType root = make_single_chain(2000); + // assigning a new value destroys the old one in place + root = nullptr; + CHECK(root.is_null()); + } +} + DOCTEST_CLANG_SUPPRESS_WARNING_POP