do not reserve preemptively in object

Author: grisumbras

include/boost/json/impl/object.hpp

@@ -376,7 +376,7 @@ insert(P&& p) ->
 {
     key_value_pair v(
         std::forward<P>(p), sp_);
-    return insert_impl(pilfer(v));
+    return emplace_impl( v.key(), pilfer(v) );
 }
 
 template<class M>
@@ -386,18 +386,14 @@ insert_or_assign(
     string_view key, M&& m) ->
         std::pair<iterator, bool>
 {
-    reserve(size() + 1);
-    auto const result = detail::find_in_object(*this, key);
-    if(result.first)
+    std::pair<iterator, bool> result = emplace_impl(
+        key, key, static_cast<M&&>(m) );
+    if( !result.second )
     {
-        value(std::forward<M>(m),
-            sp_).swap(result.first->value());
-        return { result.first, false };
+        value(static_cast<M>(m), sp_).swap(
+            result.first->value());
     }
-    key_value_pair kv(key,
-        std::forward<M>(m), sp_);
-    return { insert_impl(pilfer(kv),
-        result.second), true };
+    return result;
 }
 
 template<class Arg>
@@ -408,14 +404,7 @@ emplace(
     Arg&& arg) ->
         std::pair<iterator, bool>
 {
-    reserve(size() + 1);
-    auto const result = detail::find_in_object(*this, key);
-    if(result.first)
-        return { result.first, false };
-    key_value_pair kv(key,
-        std::forward<Arg>(arg), sp_);
-    return { insert_impl(pilfer(kv),
-        result.second), true };
+    return emplace_impl( key, key, static_cast<Arg&&>(arg) );
 }
 
 //----------------------------------------------------------
@@ -512,6 +501,40 @@ insert(
     r.commit();
 }
 
+template< class... Args >
+std::pair<object::iterator, bool>
+object::
+emplace_impl( string_view key, Args&& ... args )
+{
+    std::pair<iterator, std::size_t> search_result(nullptr, 0);
+    if( !empty() )
+    {
+        search_result = detail::find_in_object(*this, key);
+        if( search_result.first )
+            return { search_result.first, false };
+    }
+
+    // we create the new value before reserving, in case it is a reference to
+    // a subobject of the current object
+    key_value_pair kv( static_cast<Args&&>(args)..., sp_ );
+    // the key might get deallocated too
+    key = kv.key();
+
+    std::size_t const old_capacity = capacity();
+    reserve(size() + 1);
+    if( (empty() && capacity() > detail::small_object_size_)
+            || (capacity() != old_capacity) )
+        search_result.second = detail::digest(
+            key.begin(), key.end(), t_->salt);
+
+    BOOST_ASSERT(
+        t_->is_small() ||
+        (search_result.second ==
+            detail::digest(key.begin(), key.end(), t_->salt)) );
+
+    return { insert_impl(pilfer(kv), search_result.second), true };
+}
+
 //----------------------------------------------------------
 
 namespace detail {

include/boost/json/impl/object.ipp

@@ -683,23 +683,6 @@ if_contains(
 //
 //----------------------------------------------------------
 
-auto
-object::
-insert_impl(
-    pilfered<key_value_pair> p) ->
-        std::pair<iterator, bool>
-{
-    // caller is responsible
-    // for preventing aliasing.
-    reserve(size() + 1);
-    auto const result =
-        detail::find_in_object(*this, p.get().key());
-    if(result.first)
-        return { result.first, false };
-    return { insert_impl(
-        p, result.second), true };
-}
-
 key_value_pair*
 object::
 insert_impl(

include/boost/json/object.hpp

@@ -1596,10 +1596,9 @@ class object
         InputIt last,
         std::forward_iterator_tag);
 
-    BOOST_JSON_DECL
+    template< class... Args >
     std::pair<iterator, bool>
-    insert_impl(
-        pilfered<key_value_pair> p);
+    emplace_impl(string_view key, Args&& ... args );
 
     BOOST_JSON_DECL
     key_value_pair*

test/object.cpp

@@ -1595,6 +1595,33 @@ class object_test
             object({{"b",2}, {"c",3}})));
     }
 
+    void
+    testStrongGurantee()
+    {
+        // We used to preemptively reserve storage even if we don't add a new
+        // element. That violated strong guarantee requirement. This test
+        // checks we don't do that any more.
+
+        object o;
+        o.reserve(100);
+        std::size_t const capacity = o.capacity();
+        for( std::size_t i = 0; i < o.capacity() ; ++i )
+            o.emplace( std::to_string(i), i );
+        BOOST_ASSERT( capacity == o.capacity() );
+
+        BOOST_TEST( !o.emplace("0", 0).second );
+        BOOST_TEST( capacity == o.capacity() );
+
+        BOOST_TEST( !o.insert_or_assign("0", 0).second );
+        BOOST_TEST( capacity == o.capacity() );
+
+        o["0"] = 0;
+        BOOST_TEST( capacity == o.capacity() );
+
+        o.insert( key_value_pair("0", nullptr) );
+        BOOST_TEST( capacity == o.capacity() );
+    }
+
     void
     run()
     {
@@ -1610,6 +1637,7 @@ class object_test
         testEquality();
         testAllocation();
         testHash();
+        testStrongGurantee();
     }
 };