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

Enhance the flexibility of the BinaryOutputArchive and BinaryInputArchive #267

Merged
merged 3 commits into from
Jan 19, 2025

Conversation

tang-hi
Copy link
Contributor

@tang-hi tang-hi commented Jan 18, 2025

This pull request enhances the flexibility of the BinaryOutputArchive and BinaryInputArchive classes by enabling them to operate on a wider range of stream types.

Currently, BinaryOutputArchive can only serialize a single map object to a file. This restriction prevents users from storing a combination of data types within the same file, such as a map alongside integers, floats, and strings.

…am and std::istream, allowing for more flexible output/input stream handling
Copy link
Owner

@greg7mdp greg7mdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the change @tang-hi , that's a good one!

Instead of the need_close_, can you add a member std::function<void()> destruct_, and in the destructor call:

if (destruct_) destruct_();

It won't need to be initialized in the old constructor, and in the new one you can do:

    destruct_ = [this]() { delete os_; };

parallel_hashmap/phmap_dump.h Outdated Show resolved Hide resolved
parallel_hashmap/phmap_dump.h Outdated Show resolved Hide resolved
parallel_hashmap/phmap_dump.h Outdated Show resolved Hide resolved
@tang-hi
Copy link
Contributor Author

tang-hi commented Jan 19, 2025

Thanks for the reply @greg7mdp. I've updated the code based on your feedback

@greg7mdp greg7mdp merged commit f358021 into greg7mdp:master Jan 19, 2025
10 checks passed
@greg7mdp
Copy link
Owner

Thanks again for the change @tang-hi

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