Skip to content

Conversation

@lordofdestiny
Copy link

Subject

As reported in this issue ( #865 ), deserialization of the std::multimap and std::multiset breaks the container invariants. These containers are expected ( by the standard ), to maintain the relative (insertion) order of the values who's keys evaluate as equivalent.

The load method for these containers uses insert_hint(hint, value) ( or emplace_hint ) methods to add values to the container during load. This results in reversing the order of the elements with equivalent keys, because the method places the new element before the hint instead of after.

Solution

Implement handlers for these method separately instead of generically. Regular emplace is needed in order to maintain the insertion order of equivalent keys. For keys that are not equivalent there is no expectation of order so they can be placed in any order. In order to still maintain the benefits of the faster emplace_hint, it is still used when the current key is not equivalent to the previous. If the current key is equivalent to the previous key, emplace needs to be used.

Testing

By running the following cmake commands old I asserted that the current behaviour was not broken:

> cmake -DCMAKE_BUILD_TYPE=Debug -DSKIP_PERFORMANCE_COMPARISON=YES -B build -DSKIP_PORTABILITY_TEST=TRUE -DCMAKE_CXX_STANDARD=17
> cmake --build build
> cmake --build build --target test

The new behaviour was tested using the script given in the original issue.

Compilation issues

The cmake build seems to be broken in some files. In order to build the test at all the following diffs were applied:

diff --git a/include/cereal/details/helpers.hpp b/include/cereal/details/helpers.hpp
index 8dafb8e..0f058ac 100644
--- a/include/cereal/details/helpers.hpp
+++ b/include/cereal/details/helpers.hpp
@@ -244,8 +244,6 @@ namespace cereal
       static_assert( !std::is_base_of<detail::DeferredDataCore, T>::value,
                      "Cannot defer DeferredData" );
 
-      DeferredData & operator=( DeferredData const & ) = delete;
-
     public:
       //! Constructs a new NameValuePair
       /*! @param v The value to defer.  Ideally this should be an l-value reference so that
diff --git a/include/cereal/types/tuple.hpp b/include/cereal/types/tuple.hpp
index 80c6807..d690199 100644
--- a/include/cereal/types/tuple.hpp
+++ b/include/cereal/types/tuple.hpp
@@ -95,7 +95,7 @@ namespace cereal
       template <class Archive, class ... Types> inline
       static void apply( Archive & ar, std::tuple<Types...> & tuple )
       {
-        serialize<Height - 1>::template apply( ar, tuple );
+        serialize<Height - 1>::apply( ar, tuple );
         ar( CEREAL_NVP_(tuple_element_name<Height - 1>::c_str(),
             std::get<Height - 1>( tuple )) );
       }
@@ -116,7 +116,7 @@ namespace cereal
   template <class Archive, class ... Types> inline
   void CEREAL_SERIALIZE_FUNCTION_NAME( Archive & ar, std::tuple<Types...> & tuple )
   {
-    tuple_detail::serialize<std::tuple_size<std::tuple<Types...>>::value>::template apply( ar, tuple );
+    tuple_detail::serialize<std::tuple_size<std::tuple<Types...>>::value>:: apply( ar, tuple );
   }
 } // namespace cereal
diff --git a/unittests/doctest.h b/unittests/doctest.h
index cd5b44d..307315f 100644
--- a/unittests/doctest.h
+++ b/unittests/doctest.h
@@ -3631,7 +3631,7 @@ String toString(double long in) { return fpToString(in, 15); }
#define DOCTEST_TO_STRING_OVERLOAD(type, fmt)                                                      \
    String toString(type in) {                                                                     \
        char buf[64];                                                                              \
-        std::sprintf(buf, fmt, in);                                                                \
+        std::snprintf(buf, 64, fmt, in);                                                           \
        return buf;                                                                                \
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant