amorfo77/json
1
0
Fork 0
mirror of https://github.com/nlohmann/json.git synced 2026-10-05 22:20:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

Refactor destroy() for readability and add edge-case tests

Browse Source 
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 <mail@nlohmann.me>
This commit is contained in:
Niels Lohmann
2026-10-05 09:02:03 +02:00
parent 77fccaf0e7
commit daad972cdb
4 changed files with 369 additions and 188 deletions
Download Patch File Download Diff File Expand all files Collapse all files
include/nlohmann/json.hpp
+121 -94
View File
@@ -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<array_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, v.m_data.m_value.array);
std::allocator_traits<decltype(alloc)>::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<object_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, v.m_data.m_value.object);
std::allocator_traits<decltype(alloc)>::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<string_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, string);
std::allocator_traits<decltype(alloc)>::deallocate(alloc, string, 1);
}
void destroy_binary() noexcept
{
if (binary == nullptr)
{
// not initialized (e.g., due to exception in the ctor)
return;
}
AllocatorType<binary_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, binary);
std::allocator_traits<decltype(alloc)>::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<string_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, string);
std::allocator_traits<decltype(alloc)>::deallocate(alloc, string, 1);
destroy_string();
break;
}
case value_t::binary:
{
AllocatorType<binary_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, binary);
std::allocator_traits<decltype(alloc)>::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;
}
}
}
};
single_include/nlohmann/json.hpp
+121 -94
View File
@@ -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<array_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, v.m_data.m_value.array);
std::allocator_traits<decltype(alloc)>::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<object_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, v.m_data.m_value.object);
std::allocator_traits<decltype(alloc)>::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<string_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, string);
std::allocator_traits<decltype(alloc)>::deallocate(alloc, string, 1);
}
void destroy_binary() noexcept
{
if (binary == nullptr)
{
// not initialized (e.g., due to exception in the ctor)
return;
}
AllocatorType<binary_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, binary);
std::allocator_traits<decltype(alloc)>::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<string_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, string);
std::allocator_traits<decltype(alloc)>::deallocate(alloc, string, 1);
destroy_string();
break;
}
case value_t::binary:
{
AllocatorType<binary_t> alloc;
std::allocator_traits<decltype(alloc)>::destroy(alloc, binary);
std::allocator_traits<decltype(alloc)>::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;
}
}
}
};
tests/src/unit-allocator.cpp
+16
View File
@@ -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);
}
}
tests/src/unit-regression2.cpp
+111
View File
@@ -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<class BasicJsonType>
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<class BasicJsonType>
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<BasicJsonType>(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<BasicJsonType>(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<BasicJsonType>(2000);
// assigning a new value destroys the old one in place
root = nullptr;
CHECK(root.is_null());
}
}
DOCTEST_CLANG_SUPPRESS_WARNING_POP