boostorg / serialization

Boost.org serialization module
http://boost.org/libs/serialization
119 stars 139 forks source link

GitHub 267 #269

Closed schorsch1976 closed 8 months ago

schorsch1976 commented 1 year ago

Hello,

Here is a crafted pull request for review. I created a test to test the serializations (test_string.cpp) but there i also encountered some failure (line 144 test_string.cpp). As far as i understand these failure is not dependent on my changes.

https://github.com/schorsch1976/serialization/blob/github-267/test/test_string.cpp#L144

My current implementation is a copy enclosed in BOOST_NO_CXX17_HDR_MEMORY_RESOURCE with changed signatures.

Please let me know any ideas on the code.

robertramey commented 1 year ago

What is the purpose of these changes? It looks like it's mean to replace std::string to std::pmr string. Is that it? what advantage would that be. What if one saves data to an archive on a compiler pre C++17 and tries to load it with code compiled with a C++17+ compiler? Would that work? It's not obvious here. Lots of questions. Need more narrative explanation

robertramey commented 1 year ago

FYI - test_string works fine with all compilers as far as I know.

schorsch1976 commented 1 year ago

The purpose of this change is that you can change the string representation in your code to std::pmr::string and have the same binary representation without any changes.

struct s // old one
{
    std::string msg;
    template<class Archive>
    void serialize(Archive & ar, const unsigned int version)
    {
        ar & BOOST_SERIALIZATION_NVP(msg);
    }
};
struct s // new one
{
    std::pmr::string msg;
    template<class Archive>
    void serialize(Archive & ar, const unsigned int version)
    {
        ar & BOOST_SERIALIZATION_NVP(msg);
    }
};

would both serialize into/from <msg>mytext</msg>. As they serialize into the same representation, you can serialize with a c++17 library and read it with a c++03 library and vica verse.

When you look at my change, you see that i adopted the function signature but the same implementation and of course the compilation with when compiled in c++17 mode.

https://github.com/schorsch1976/serialization/commit/1a0ab95594da0a818134308d04dd43e491c01d00

What i did to make it working with std::pmr::string on my current code base is a wrapper that can accomplish my desired behaviour but at the cost of a different serialization format as boost::serialization handles std::string and std::pmr::string differently as totally different types. I did this in my project as this change is not upstream and i don't want to carry a fork of boost. I would prefer to bring it upstream. Different projects could then just switch their representation and be done with it.

Hope this makes it more clear what the purpose is.

robertramey commented 1 year ago

I spent some time looking into this.

I'm thinking you're not thinking big enough. I think the correct way to consider this is for the collection serialization code to be enhanced so it takes any allocator via a template parameter. That way ALL the code for present and future collections could be serialized without modifying files all over the place - which is of course very error prone.

And there is the line in the test code

// fails but why? //result += PerformTest<std::wstring, boost::archive::xml_oarchive, boost::archive::xml_iarchive>(); //result += PerformTest<std::pmr::wstring, boost::archive::xml_oarchive, boost::archive::xml_iarchive>();

I think this is worthy idea, but needs more thought.

robertramey commented 1 year ago

And change the name of the PR to something more useful such as "extend collection serialization to all allocators"

schorsch1976 commented 1 year ago

I take a look into it.