Merge branch 'develop' into claude/deep-copy-key-comp-5649

Conflicts:
- tests/src/unit-comparison.cpp: kept both sides' helpers (key_case_less/
  key_case_json/innermost from the PR, case_insensitive_less/ci_json from
  develop's #5720) and both new test cases

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

docs/mkdocs/docs/api/basic_json/emplace.md

@@ -69,5 +69,3 @@ Logarithmic in the size of the container, O(log(`size()`)).
 ## Version history
 
 - Since version 2.0.8.
-- Fixed in version 3.13.0: for [`ordered_json`](../ordered_json.md), the value could previously only be passed as an
-  rvalue; it can now also be passed as an lvalue or a `#!cpp const` lvalue, matching the behavior of `json`.

include/nlohmann/detail/meta/type_traits.hpp

@@ -189,6 +189,37 @@ struct actual_object_comparator
 template<typename BasicJsonType>
 using actual_object_comparator_t = typename actual_object_comparator<BasicJsonType>::type;
 
+template<typename T>
+using detect_key_comp = decltype(std::declval<const T&>().key_comp());
+
+// whether ObjectType can be constructed from a pair of Iterator together with
+// a copy of its own comparator, the way std::map can: it needs a nested
+// key_compare, a const key_comp() convertible to it, and a matching
+// (Iterator, Iterator, const key_compare&) constructor.
+//
+// used to preserve a stateful comparator when a copy is built from a range
+// past the iterative deep copy's nesting bound (see copy_object_level); an
+// object type that does not satisfy this, such as nlohmann::ordered_map
+// (which has key_compare for its std::map-like interface, but no key_comp()),
+// keeps default-constructing its comparator, just as it always has
+template<typename ObjectType, typename Iterator, typename = void>
+struct is_comparator_constructible_object_type_impl : std::false_type {};
+
+template<typename ObjectType, typename Iterator>
+struct is_comparator_constructible_object_type_impl <
+    ObjectType, Iterator, enable_if_t<is_detected<detect_key_compare, ObjectType>::value >>
+{
+    using key_compare = typename ObjectType::key_compare;
+
+    static constexpr bool value =
+        is_detected_convertible<key_compare, detect_key_comp, ObjectType>::value &&
+        std::is_constructible<ObjectType, Iterator, Iterator, const key_compare&>::value;
+};
+
+template<typename ObjectType, typename Iterator>
+struct is_comparator_constructible_object_type
+    : is_comparator_constructible_object_type_impl<ObjectType, Iterator> {};
+
 /////////////////
 // char_traits //
 /////////////////

include/nlohmann/json.hpp

@@ -1127,6 +1127,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
         }
     }
 
+    /// @brief create the object type from a range, preserving @a src_object's
+    /// comparator when the object type supports it
+    /// Enabled for object types that provide a key_comp() and a matching
+    /// range-plus-comparator constructor, such as std::map. Other object
+    /// types, such as nlohmann::ordered_map, fall back to the plain range
+    /// constructor and default-construct their comparator, just as they
+    /// always have (@ref detail::is_comparator_constructible_object_type).
+    template<typename Iterator, detail::enable_if_t<
+                 detail::is_comparator_constructible_object_type<object_t, Iterator>::value, int> = 0>
+    static object_t* create_object_with_comparator(const object_t& src_object, Iterator first, Iterator last)
+    {
+        return create<object_t>(first, last, src_object.key_comp());
+    }
+
+    template<typename Iterator, detail::enable_if_t<
+                 detail::negation<detail::is_comparator_constructible_object_type<object_t, Iterator>>::value, int> = 0>
+    static object_t* create_object_with_comparator(const object_t& /*src_object*/, Iterator first, Iterator last)
+    {
+        return create<object_t>(first, last);
+    }
+
     /// @brief create the copy of the object @a src in @a dst
     /// @note structured values are appended to @a worklist instead
     static void copy_object_level(const basic_json& src, basic_json& dst,
@@ -1144,7 +1165,8 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
             scratch.emplace_back(element.first, basic_json());
         }
 
-        dst.m_data.m_value.object = create<object_t>(std::make_move_iterator(scratch.begin()),
+        dst.m_data.m_value.object = create_object_with_comparator(src_object,
+                                    std::make_move_iterator(scratch.begin()),
                                     std::make_move_iterator(scratch.end()));
         // only now that the object exists may dst stop being a null value
         dst.m_data.m_type = value_t::object;

include/nlohmann/ordered_map.hpp

@@ -72,9 +72,7 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
         return *this;
     }
 
-    template<class V, detail::enable_if_t<
-                 detail::is_constructible<T, V>::value, int> = 0>
-    std::pair<iterator, bool> emplace(const key_type& key, V && t)
+    std::pair<iterator, bool> emplace(const key_type& key, T&& t)
     {
         for (auto it = this->begin(); it != this->end(); ++it)
         {
@@ -83,14 +81,13 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                 return {it, false};
             }
         }
-        append(key, std::forward<V>(t));
+        append(key, std::forward<T>(t));
         return {std::prev(this->end()), true};
     }
 
-    template<class KeyType, class V, detail::enable_if_t<
-                 detail::conjunction<detail::is_usable_as_key_type<key_compare, key_type, KeyType>,
-                                     detail::is_constructible<T, V>>::value, int> = 0>
-    std::pair<iterator, bool> emplace(KeyType && key, V && t)
+    template<class KeyType, detail::enable_if_t<
+                 detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
+    std::pair<iterator, bool> emplace(KeyType && key, T && t)
     {
         for (auto it = this->begin(); it != this->end(); ++it)
         {
@@ -99,7 +96,7 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                 return {it, false};
             }
         }
-        append(std::forward<KeyType>(key), std::forward<V>(t));
+        append(std::forward<KeyType>(key), std::forward<T>(t));
         return {std::prev(this->end()), true};
     }
 

single_include/nlohmann/json.hpp

@@ -4197,6 +4197,37 @@ struct actual_object_comparator
 template<typename BasicJsonType>
 using actual_object_comparator_t = typename actual_object_comparator<BasicJsonType>::type;
 
+template<typename T>
+using detect_key_comp = decltype(std::declval<const T&>().key_comp());
+
+// whether ObjectType can be constructed from a pair of Iterator together with
+// a copy of its own comparator, the way std::map can: it needs a nested
+// key_compare, a const key_comp() convertible to it, and a matching
+// (Iterator, Iterator, const key_compare&) constructor.
+//
+// used to preserve a stateful comparator when a copy is built from a range
+// past the iterative deep copy's nesting bound (see copy_object_level); an
+// object type that does not satisfy this, such as nlohmann::ordered_map
+// (which has key_compare for its std::map-like interface, but no key_comp()),
+// keeps default-constructing its comparator, just as it always has
+template<typename ObjectType, typename Iterator, typename = void>
+struct is_comparator_constructible_object_type_impl : std::false_type {};
+
+template<typename ObjectType, typename Iterator>
+struct is_comparator_constructible_object_type_impl <
+    ObjectType, Iterator, enable_if_t<is_detected<detect_key_compare, ObjectType>::value >>
+{
+    using key_compare = typename ObjectType::key_compare;
+
+    static constexpr bool value =
+        is_detected_convertible<key_compare, detect_key_comp, ObjectType>::value &&
+        std::is_constructible<ObjectType, Iterator, Iterator, const key_compare&>::value;
+};
+
+template<typename ObjectType, typename Iterator>
+struct is_comparator_constructible_object_type
+    : is_comparator_constructible_object_type_impl<ObjectType, Iterator> {};
+
 /////////////////
 // char_traits //
 /////////////////
@@ -26622,9 +26653,7 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
         return *this;
     }
 
-    template<class V, detail::enable_if_t<
-                 detail::is_constructible<T, V>::value, int> = 0>
-    std::pair<iterator, bool> emplace(const key_type& key, V && t)
+    std::pair<iterator, bool> emplace(const key_type& key, T&& t)
     {
         for (auto it = this->begin(); it != this->end(); ++it)
         {
@@ -26633,14 +26662,13 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                 return {it, false};
             }
         }
-        append(key, std::forward<V>(t));
+        append(key, std::forward<T>(t));
         return {std::prev(this->end()), true};
     }
 
-    template<class KeyType, class V, detail::enable_if_t<
-                 detail::conjunction<detail::is_usable_as_key_type<key_compare, key_type, KeyType>,
-                                     detail::is_constructible<T, V>>::value, int> = 0>
-    std::pair<iterator, bool> emplace(KeyType && key, V && t)
+    template<class KeyType, detail::enable_if_t<
+                 detail::is_usable_as_key_type<key_compare, key_type, KeyType>::value, int> = 0>
+    std::pair<iterator, bool> emplace(KeyType && key, T && t)
     {
         for (auto it = this->begin(); it != this->end(); ++it)
         {
@@ -26649,7 +26677,7 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                 return {it, false};
             }
         }
-        append(std::forward<KeyType>(key), std::forward<V>(t));
+        append(std::forward<KeyType>(key), std::forward<T>(t));
         return {std::prev(this->end()), true};
     }
 
@@ -28057,6 +28085,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
         }
     }
 
+    /// @brief create the object type from a range, preserving @a src_object's
+    /// comparator when the object type supports it
+    /// Enabled for object types that provide a key_comp() and a matching
+    /// range-plus-comparator constructor, such as std::map. Other object
+    /// types, such as nlohmann::ordered_map, fall back to the plain range
+    /// constructor and default-construct their comparator, just as they
+    /// always have (@ref detail::is_comparator_constructible_object_type).
+    template<typename Iterator, detail::enable_if_t<
+                 detail::is_comparator_constructible_object_type<object_t, Iterator>::value, int> = 0>
+    static object_t* create_object_with_comparator(const object_t& src_object, Iterator first, Iterator last)
+    {
+        return create<object_t>(first, last, src_object.key_comp());
+    }
+
+    template<typename Iterator, detail::enable_if_t<
+                 detail::negation<detail::is_comparator_constructible_object_type<object_t, Iterator>>::value, int> = 0>
+    static object_t* create_object_with_comparator(const object_t& /*src_object*/, Iterator first, Iterator last)
+    {
+        return create<object_t>(first, last);
+    }
+
     /// @brief create the copy of the object @a src in @a dst
     /// @note structured values are appended to @a worklist instead
     static void copy_object_level(const basic_json& src, basic_json& dst,
@@ -28074,7 +28123,8 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
             scratch.emplace_back(element.first, basic_json());
         }
 
-        dst.m_data.m_value.object = create<object_t>(std::make_move_iterator(scratch.begin()),
+        dst.m_data.m_value.object = create_object_with_comparator(src_object,
+                                    std::make_move_iterator(scratch.begin()),
                                     std::make_move_iterator(scratch.end()));
         // only now that the object exists may dst stop being a null value
         dst.m_data.m_type = value_t::object;

tests/src/unit-comparison.cpp

@@ -827,6 +827,46 @@ Json nest(Json j, const std::size_t depth)
     return j;
 }
 
+// a std::map comparator with state: case-insensitive, unless constructed
+// case-sensitive. Used to check that copying an object copies the original's
+// comparator rather than default-constructing a new one (see #5649).
+struct key_case_less
+{
+    key_case_less() = default;
+    explicit key_case_less(const bool cs) noexcept : case_sensitive(cs) {}
+
+    bool operator()(const std::string& a, const std::string& b) const
+    {
+        if (case_sensitive)
+        {
+            return a < b;
+        }
+        return std::lexicographical_compare(a.begin(), a.end(), b.begin(), b.end(),
+                                            [](unsigned char x, unsigned char y)
+        {
+            return std::tolower(x) < std::tolower(y);
+        });
+    }
+
+    bool case_sensitive = false;
+};
+
+template<class Key, class Value, class /*Compare*/, class Allocator>
+using key_case_map = std::map<Key, Value, key_case_less, Allocator>;
+using key_case_json = nlohmann::basic_json<key_case_map>;
+
+// the innermost value of a chain of single-element arrays
+template<typename Json>
+const Json& innermost(const Json& j)
+{
+    const Json* p = &j;
+    while (p->is_array())
+    {
+        p = &(*p)[0];
+    }
+    return *p;
+}
+
 // orders keys case-insensitively, so "key" and "KEY" compare equivalent
 // (neither less than the other) although they are not equal
 struct case_insensitive_less
@@ -891,6 +931,47 @@ TEST_CASE("equality of objects whose entries have no fixed order")
     }
 }
 
+TEST_CASE("copying an object preserves its comparator's state")
+{
+    // Past the iterative deep copy's nesting bound, an object copy used to be
+    // built with a default-constructed comparator instead of a copy of the
+    // original's. For an object type whose comparator carries state - here, a
+    // std::map that compares keys case-sensitively only when created that way
+    // - this reordered the copy's keys and could even drop entries that the
+    // original's comparator kept distinct (see #5649).
+    key_case_json object = key_case_json::object_t(key_case_less(true)); // case-sensitive
+    object["b"] = 1;
+    object["B"] = 2;
+    object["a"] = 3;
+    REQUIRE(object.dump() == R"({"B":2,"a":3,"b":1})");
+
+    for (const std::size_t depth : std::vector<std::size_t> {0, 127, 128, 200})
+    {
+        CAPTURE(depth);
+
+        key_case_json original = object;
+        for (std::size_t i = 0; i < depth; ++i)
+        {
+            original = key_case_json::array({std::move(original)});
+        }
+
+        {
+            const key_case_json copy = original; // NOLINT(performance-unnecessary-copy-initialization)
+            CHECK(innermost(copy).size() == 3);
+            CHECK(innermost(copy).dump() == R"({"B":2,"a":3,"b":1})");
+            CHECK(copy == original);
+        }
+
+        {
+            key_case_json copy = key_case_json::array();
+            copy = original;
+            CHECK(innermost(copy).size() == 3);
+            CHECK(innermost(copy).dump() == R"({"B":2,"a":3,"b":1})");
+            CHECK(copy == original);
+        }
+    }
+}
+
 TEST_CASE("equality of an object whose comparator treats different keys as equivalent")
 {
     // https://github.com/nlohmann/json/issues/5655: past the nesting bound,

tests/src/unit-ordered_json.cpp

@@ -196,44 +196,3 @@ TEST_CASE("regression test - diff() must account for ordered_json member order")
         CHECK(a.patch(p) == b);
     }
 }
-
-TEST_CASE("regression test for issue #5673 - ordered_json::emplace with a non-rvalue value")
-{
-    SECTION("lvalue value")
-    {
-        ordered_json oj = ordered_json::object();
-        ordered_json value = 1;
-        auto res = oj.emplace("a", value);
-        CHECK(res.second == true);
-        CHECK(oj.dump() == "{\"a\":1}");
-    }
-
-    SECTION("const lvalue value")
-    {
-        ordered_json oj = ordered_json::object();
-        const ordered_json value = 1;
-        auto res = oj.emplace("a", value);
-        CHECK(res.second == true);
-        CHECK(oj.dump() == "{\"a\":1}");
-    }
-
-    SECTION("rvalue value")
-    {
-        ordered_json oj = ordered_json::object();
-        auto res = oj.emplace("a", ordered_json(1));
-        CHECK(res.second == true);
-        CHECK(oj.dump() == "{\"a\":1}");
-    }
-
-    SECTION("existing key is not overwritten (std::map-compatible semantics)")
-    {
-        ordered_json oj = ordered_json::object();
-        ordered_json value = 1;
-        oj.emplace("a", value);
-
-        ordered_json other_value = 2;
-        auto res = oj.emplace("a", other_value);
-        CHECK(res.second == false);
-        CHECK(oj.dump() == "{\"a\":1}");
-    }
-}

tests/src/unit-ordered_map.cpp

@@ -403,79 +403,6 @@ TEST_CASE("ordered_map")
             CHECK(om.size() == 4);
         }
     }
-
-    SECTION("emplace")
-    {
-        // regression test for issue #5673: the mapped-value parameter must
-        // accept lvalues and const lvalues, not just rvalues
-        ordered_map<std::string, std::string> om;
-        om["eins"] = "one";
-        om["zwei"] = "two";
-        om["drei"] = "three";
-
-        SECTION("with T&& (rvalue)")
-        {
-            auto res1 = om.emplace("eins", std::string("1"));
-            CHECK(res1.first == om.begin());
-            CHECK(res1.second == false);
-            CHECK(om.size() == 3);
-            CHECK(om.at("eins") == "one"); // existing key is not overwritten
-
-            auto res4 = om.emplace("vier", std::string("four"));
-            CHECK(res4.first == om.begin() + 3);
-            CHECK(res4.second == true);
-            CHECK(om.size() == 4);
-            CHECK(om.at("vier") == "four");
-        }
-
-        SECTION("with T& (lvalue)")
-        {
-            std::string one = "1";
-            std::string four = "four";
-
-            auto res1 = om.emplace("eins", one);
-            CHECK(res1.first == om.begin());
-            CHECK(res1.second == false);
-            CHECK(om.size() == 3);
-            CHECK(om.at("eins") == "one"); // existing key is not overwritten
-
-            auto res4 = om.emplace("vier", four);
-            CHECK(res4.first == om.begin() + 3);
-            CHECK(res4.second == true);
-            CHECK(om.size() == 4);
-            CHECK(om.at("vier") == "four");
-            CHECK(four == "four"); // source was copied, not moved from
-        }
-
-        SECTION("with const T&")
-        {
-            const std::string one = "1";
-            const std::string four = "four";
-
-            auto res1 = om.emplace("eins", one);
-            CHECK(res1.first == om.begin());
-            CHECK(res1.second == false);
-            CHECK(om.size() == 3);
-
-            auto res4 = om.emplace("vier", four);
-            CHECK(res4.first == om.begin() + 3);
-            CHECK(res4.second == true);
-            CHECK(om.size() == 4);
-            CHECK(om.at("vier") == "four");
-        }
-
-        SECTION("with key of key_type (non-template overload)")
-        {
-            const std::string key_vier{"vier"};
-            std::string four = "four";
-
-            auto res4 = om.emplace(key_vier, four);
-            CHECK(res4.first == om.begin() + 3);
-            CHECK(res4.second == true);
-            CHECK(om.size() == 4);
-            CHECK(om.at("vier") == "four");
-        }
-    }
 }
 
 TEST_CASE("ordered_map growth")