Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove serialization support from Boost.Unordered containers #289

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

cmazakas
Copy link
Member

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:
boostorg/unordered#207

@robertramey
Copy link
Member

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
Copy link
Member Author

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.");

Unordered now supports its Serialization functions so the headers just need to be updated to keep backwards compatibility
@cmazakas
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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.

@robertramey robertramey merged commit 404e0a3 into boostorg:develop Oct 13, 2023
15 of 38 checks passed
@cmazakas cmazakas deleted the unordered-serialization branch October 14, 2023 00:32
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.

2 participants