From 7192763f15a0bec10fc72dcc16cf9c327bcdae65 Mon Sep 17 00:00:00 2001 From: SamareshSingh <97642706+ssam18@users.noreply.github.com> Date: Thu, 14 May 2026 01:54:24 -0500 Subject: [PATCH] Fix compile error when using nlohmann ordered_map with WITH_DEFAULT macros (#5163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix compile error when using nlohmann ordered_map with WITH_DEFAULT macros ordered_map inherits its copy and move assignment from the underlying std vector, which requires value_type to be CopyAssignable. value_type is pair whose assignment is deleted because of the const Key, so any code that assigns ordered_map (for example the ternary in NLOHMANN_JSON_FROM_WITH_DEFAULT) fails to compile (issue #5122). Provide assignment operators on ordered_map that rebuild via clear plus push_back for copy and transfer the underlying buffer for move, neither of which needs pair assignment. Also switch the map-shaped from_json overload from a transform plus inserter idiom to a range-for plus emplace, which avoids the same hazard. Signed-off-by: Samaresh Kumar Singh * Update ordered_map.hpp removed unwanted comments Signed-off-by: SamareshSingh <97642706+ssam18@users.noreply.github.com> Signed-off-by: Samaresh Kumar Singh * Update json.hpp Signed-off-by: SamareshSingh <97642706+ssam18@users.noreply.github.com> Signed-off-by: Samaresh Kumar Singh * Address CI issues for ordered_map fix Declare an explicit defaulted destructor on ordered_map so the rule of five is complete (clang-tidy cppcoreguidelines-special-member-functions and hicpp-special-member-functions). Initialize the ordered_map field in the regression test struct so GCC effective-C++ stops flagging Example_5122 with a missing member initializer. Signed-off-by: Samaresh Kumar Singh * Suppress redundant-member-init lint on Example_5122::c The empty brace-init on c{} is required by GCC -Weffc++ to mark the member as initialized in the synthesized default constructor, but clang-tidy readability-redundant-member-init flags the same line because ordered_map already has a default constructor. The two checks pull in opposite directions, so add a targeted NOLINT to keep both happy. Signed-off-by: Samaresh Kumar Singh * Address review: strong exception safety in copy-assign, simplify move-assign noexcept Copy assignment now constructs a temporary copy before move-assigning the Container subobject, preserving *this if the copy throws. Move assignment uses std::is_nothrow_move_assignable for a cleaner noexcept specifier, matching the style of the move constructor. Signed-off-by: Samaresh Kumar Singh * Restore self-assignment check in copy-assign to satisfy cert-oop54-cpp clang-tidy's cert-oop54-cpp flagged the previous revision because it could not recognize the implicit self-safety of the copy-then-move pattern. Restore the explicit `if (this != &other)` guard — strong exception safety is preserved since the temporary copy is still constructed before the move-assign of the Container subobject. Signed-off-by: Samaresh Kumar Singh * Address review: add explicit self-assignment and move-assignment tests for ordered_map Signed-off-by: Samaresh Kumar Singh * Address review: gate -Wself-assign-overloaded suppression on Clang version -Wself-assign-overloaded was introduced in Clang 7. Older Clang versions fail the build with "unknown warning group" when the suppression pragma references it unconditionally. Use __has_warning inside an __clang__ branch so the suppression is only emitted on Clang versions that recognize the warning. The inner check stays inside the __clang__ guard because GCC does not provide __has_warning and would tokenize-error on the argument list. Signed-off-by: Samaresh Kumar Singh * Address CI: drop unused gating macro to silence -Wunused-macros The previous attempt defined JSON_TEST_5122_SUPPRESS_SELF_ASSIGN_OVERLOADED as 0 unconditionally and then overrode it to 1 on Clang versions that recognize the warning. On those Clangs the initial define is immediately undef'd without being read, which trips Clang's -Wunused-macros under -Weverything in the ci_test_clang job. Drop the macro and gate the DOCTEST_CLANG_SUPPRESS_WARNING_PUSH/POP pragmas directly with __has_warning inside the existing __clang__ branch. Signed-off-by: Samaresh Kumar Singh --------- Signed-off-by: Samaresh Kumar Singh Signed-off-by: SamareshSingh <97642706+ssam18@users.noreply.github.com> --- .../nlohmann/detail/conversions/from_json.hpp | 10 +-- include/nlohmann/ordered_map.hpp | 19 ++++ single_include/nlohmann/json.hpp | 29 ++++-- tests/src/unit-regression2.cpp | 89 +++++++++++++++++++ 4 files changed, 133 insertions(+), 14 deletions(-) diff --git a/include/nlohmann/detail/conversions/from_json.hpp b/include/nlohmann/detail/conversions/from_json.hpp index 2a814d1e3..5a7cdb624 100644 --- a/include/nlohmann/detail/conversions/from_json.hpp +++ b/include/nlohmann/detail/conversions/from_json.hpp @@ -388,14 +388,10 @@ inline void from_json(const BasicJsonType& j, ConstructibleObjectType& obj) ConstructibleObjectType ret; const auto* inner_object = j.template get_ptr(); - using value_type = typename ConstructibleObjectType::value_type; - std::transform( - inner_object->begin(), inner_object->end(), - std::inserter(ret, ret.begin()), - [](typename BasicJsonType::object_t::value_type const & p) + for (const auto& p : *inner_object) { - return value_type(p.first, p.second.template get()); - }); + ret.emplace(p.first, p.second.template get()); + } obj = std::move(ret); } diff --git a/include/nlohmann/ordered_map.hpp b/include/nlohmann/ordered_map.hpp index d8606f82f..5ba0da18f 100644 --- a/include/nlohmann/ordered_map.hpp +++ b/include/nlohmann/ordered_map.hpp @@ -50,6 +50,25 @@ template , : Container{first, last, alloc} {} ordered_map(std::initializer_list init, const Allocator& alloc = Allocator() ) : Container{init, alloc} {} + ordered_map(const ordered_map&) = default; + ordered_map(ordered_map&&) noexcept(std::is_nothrow_move_constructible::value) = default; + ~ordered_map() = default; + + ordered_map& operator=(const ordered_map& other) + { + if (this != &other) + { + ordered_map tmp(other); + Container::operator=(std::move(static_cast(tmp))); + } + return *this; + } + + ordered_map& operator=(ordered_map&& other) noexcept(std::is_nothrow_move_assignable::value) + { + Container::operator=(std::move(static_cast(other))); + return *this; + } std::pair emplace(const key_type& key, T&& t) { diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 441b03e09..69073a496 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -5306,14 +5306,10 @@ inline void from_json(const BasicJsonType& j, ConstructibleObjectType& obj) ConstructibleObjectType ret; const auto* inner_object = j.template get_ptr(); - using value_type = typename ConstructibleObjectType::value_type; - std::transform( - inner_object->begin(), inner_object->end(), - std::inserter(ret, ret.begin()), - [](typename BasicJsonType::object_t::value_type const & p) + for (const auto& p : *inner_object) { - return value_type(p.first, p.second.template get()); - }); + ret.emplace(p.first, p.second.template get()); + } obj = std::move(ret); } @@ -20032,6 +20028,25 @@ template , : Container{first, last, alloc} {} ordered_map(std::initializer_list init, const Allocator& alloc = Allocator() ) : Container{init, alloc} {} + ordered_map(const ordered_map&) = default; + ordered_map(ordered_map&&) noexcept(std::is_nothrow_move_constructible::value) = default; + ~ordered_map() = default; + + ordered_map& operator=(const ordered_map& other) + { + if (this != &other) + { + ordered_map tmp(other); + Container::operator=(std::move(static_cast(tmp))); + } + return *this; + } + + ordered_map& operator=(ordered_map&& other) noexcept(std::is_nothrow_move_assignable::value) + { + Container::operator=(std::move(static_cast(other))); + return *this; + } std::pair emplace(const key_type& key, T&& t) { diff --git a/tests/src/unit-regression2.cpp b/tests/src/unit-regression2.cpp index 64deba448..959e30197 100644 --- a/tests/src/unit-regression2.cpp +++ b/tests/src/unit-regression2.cpp @@ -1240,4 +1240,93 @@ TEST_CASE("regression test #5074 - single-element brace init with JSON_BRACE_INI } #endif +struct Example_5122 +{ + float b = 2; + nlohmann::ordered_map c{}; // NOLINT(readability-redundant-member-init): needed for GCC -Weffc++ + int a = 1; + NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(Example_5122, b, c, a) +}; + +TEST_CASE("regression test #5122 - from_json into types holding nlohmann::ordered_map") +{ + Example_5122 src; + src.c.emplace("first", "1"); + src.c.emplace("second", "2"); + + ordered_json const j = src; + Example_5122 const dst = j.get(); + + CHECK(dst.b == src.b); + CHECK(dst.a == src.a); + REQUIRE(dst.c.size() == src.c.size()); + auto src_it = src.c.begin(); + auto dst_it = dst.c.begin(); + for (; src_it != src.c.end(); ++src_it, ++dst_it) + { + CHECK(dst_it->first == src_it->first); + CHECK(dst_it->second == src_it->second); + } +} + +// -Wself-assign-overloaded was introduced in Clang 7. Gate the pragma on +// __has_warning so older Clang versions do not error with "unknown warning +// group". The __has_warning check has to stay inside the __clang__ branch +// because GCC does not provide it and would tokenize-error on the argument. +#if defined(__clang__) && defined(__has_warning) + #if __has_warning("-Wself-assign-overloaded") +DOCTEST_CLANG_SUPPRESS_WARNING_PUSH +DOCTEST_CLANG_SUPPRESS_WARNING("-Wself-assign-overloaded") + #endif +#endif + +TEST_CASE("regression test #5122 - nlohmann::ordered_map copy-assignment is self-assignment safe") +{ + nlohmann::ordered_map m; + m.emplace("first", "1"); + m.emplace("second", "2"); + + // Insertion order is preserved by ordered_map, so we can check it directly. + m = m; + + REQUIRE(m.size() == 2); + auto it = m.begin(); + CHECK(it->first == "first"); + CHECK(it->second == "1"); + ++it; + CHECK(it->first == "second"); + CHECK(it->second == "2"); +} + +#if defined(__clang__) && defined(__has_warning) + #if __has_warning("-Wself-assign-overloaded") +DOCTEST_CLANG_SUPPRESS_WARNING_POP + #endif +#endif + +TEST_CASE("regression test #5122 - nlohmann::ordered_map move-assignment transfers contents") +{ + nlohmann::ordered_map src; + src.emplace("first", "1"); + src.emplace("second", "2"); + + nlohmann::ordered_map dst; + dst.emplace("stale", "x"); + dst = std::move(src); + + REQUIRE(dst.size() == 2); + auto it = dst.begin(); + CHECK(it->first == "first"); + CHECK(it->second == "1"); + ++it; + CHECK(it->first == "second"); + CHECK(it->second == "2"); + + // Re-assigning into the moved-from object must leave it in a usable state. + src = nlohmann::ordered_map{}; + src.emplace("after-move", "3"); + REQUIRE(src.size() == 1); + CHECK(src.begin()->first == "after-move"); +} + DOCTEST_CLANG_SUPPRESS_WARNING_POP