boostorg / serialization

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

Remove serialization support from Boost.Unordered containers #289

Closed cmazakas closed 11 months ago

cmazakas commented 1 year ago

Unordered added a slew of new containers and while adding Serialization support for them, we noticed Serialization directly supports boost::unordered_map et al.

We'd like to own our own means of serialization and to prevent breaking changes to Serialization and its users, we made this PR to maintain compatibility.

We haven't updated Unordered's develop branch yet because this would break Serialization's CI. You can see the open pull request here: https://github.com/boostorg/unordered/pull/207

robertramey commented 1 year ago

Hmmm - I don't really understand this yet. Is the new Boost.Unordered compatible with the current previous one? If it's not one would need to know what to do about files serialized under the previous version. Assuming that they are compatible, and you want to take over this functionality, I'd be all for it. But I need to know that it won't break currently existing archives.

Assuming that the above is satisfied. The real fix is to remove the files currently in the serialization library which implement and test this functionality. The current header would contain nothing but a #pragma message pointing the user to the fact that he should make adjustments in his program. Then responsibility for testing would fall entirely to you - which is fine with me. You could also just re-use the current headers boost/serialization/hash_set.hpp with your own code update and the tests would continue to run as they do now.

cmazakas commented 1 year ago

Hey Robert, thanks for taking the time look at the PR!

Hmmm - I don't really understand this yet. Is the new Boost.Unordered compatible with the current previous one? If it's not one would need to know what to do about files serialized under the previous version. Assuming that they are compatible, and you want to take over this functionality, I'd be all for it. But I need to know that it won't break currently existing archives.

Luckily, Joaquin has you covered here: https://github.com/boostorg/unordered/blob/b0195297f096c0477242bce98f4dd6209cfd0a12/test/legacy_archives/generate_legacy_archives.cpp

We have a committed set of legacy archives that we've included in our serialization tests here: https://github.com/boostorg/unordered/blob/b0195297f096c0477242bce98f4dd6209cfd0a12/test/unordered/serialization_tests.cpp#L185

Assuming that the above is satisfied. The real fix is to remove the files currently in the serialization library which implement and test this functionality. The current header would contain nothing but a #pragma message pointing the user to the fact that he should make adjustments in his program. Then responsibility for testing would fall entirely to you - which is fine with me.

Great minds think alike, I was suggesting a pragma message to Joaquin earlier.

What would you like the message to say? Something maybe just like:

BOOST_PRAGMA_MEESAGE("This header is deprecated, Unordered supports Serialization directly now.");
cmazakas commented 1 year ago

@robertramey I think this PR should be good to go, Unordered's already merged in its support for Serialization which essentially pollutes your CI with needless test failures.

robertramey commented 1 year ago

It looks to me that some of the removed files contain code which implements serialization for std hashed/unordered containers. Am I right? Are you sure you're not removing something that someone might be using?

cmazakas commented 12 months ago

It looks to me that some of the removed files contain code which implements serialization for std hashed/unordered containers. Am I right? Are you sure you're not removing something that someone might be using?

That functionality should live in include/boost/serialization/unordered_set.hpp, no?

Here I'm only updating: include/boost/serialization/boost_unordered_set.hpp.

Unless I'm missing something obvious here, everything seems okay.

robertramey commented 12 months ago

That functionality should live in include/boost/serialization/unordered_set.hpp, no?

That's my question. Isn't the point of this to totally eliminate boost/serialization/unordered_set.hpp? does the functionality to serialize std::unordered_set.hpp reside in your library?

cmazakas commented 12 months ago

Isn't the point of this to totally eliminate boost/serialization/unordered_set.hpp? does the functionality to serialize std::unordered_set.hpp reside in your library?

It does not. This PR is strictly about Boost.Unordered and not the STL containers.

Sorry, I should've introduced myself better. I'm the maintainer of Boost.Unordered and work alongside Joaquin to develop new containers for the library.

Our goal here is to only remove support for boost::unordered_* containers while leaving your support for the STL containers intact.