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

Add serialization for std::optional #163

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

sdebionne
Copy link
Contributor

Fixes #114 together with #148.

@robertramey
Copy link
Member

I don't see any tests for this. That every "built-in" type which includes serialization code must be tested. That is must have a test program written for it. It's not hard - look at the rest of them. But it is a requirement. Looking forward to this.

@sdebionne
Copy link
Contributor Author

Speaking of the tests, the CI is broken since Dec 16, 2020. The std::variant has tests but was never merged. So why should I bother?

@Ulysses1337
Copy link

What I hope to do one day is replace all my Boost optionals with std. Can we name the bool field "initialized" for compatibility?

@robertramey robertramey merged commit 1af820b into boostorg:develop Sep 3, 2023
@robertramey
Copy link
Member

I merged this and pulled into my local repo. I was considering writing a test for it. Upon closer examination I found there is an implementation for boost::optional serialization which is almost identical to your implementation of serialization for std::optional. So it seems to me that we should include serialization for both boost::optional and std::option in the same file. And the test for optional should be extended to include std::optional as well. This will diminish the code which needs to be maintained in the future.

As an aside, I think you should have credited the original author of the serialization for boost::optional in your header.

@sdebionne
Copy link
Contributor Author

Of course it is almost identical as boost and std optionals have the same interfaces.

So it seems to me that we should include serialization for both boost::optional and std::option in the same file

Why force people that use std::optional to depend on boost::optional via this shared header?

the test for optional should be extended to include std::optional as well

Agreed but given that std::variant did not get merged, I did not want to spend time on adding the test here.

As an aside, I think you should have credited the original author of the serialization for boost::optional in your header.

OK. I would create a PR for that but since it has been reverted in #299, I won't.

@robertramey
Copy link
Member

I've incorporated your working boost/serialization/optional and boost/serialization/variant. I think you'll find this satisfactory. Feel free to review it and ask me about it.

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.

add seralization for std::optional and std::variant
3 participants