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

[3.2] Fix incorrect serializing of std::optional when value is not provided - (GH #1189) #1364

Merged
merged 8 commits into from
Jul 5, 2023

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Jul 3, 2023

When serializing a std::optional field, and a value is not provided for the std::optional, we need to add into the serialization the flag specifying that a missing value is serialized. Prior to this PR, the flag was missing.

@greg7mdp greg7mdp requested a review from heifner July 3, 2023 15:43
libraries/libfc/include/fc/time.hpp Outdated Show resolved Hide resolved
unittests/abi_tests.cpp Show resolved Hide resolved
@greg7mdp greg7mdp requested a review from linh2931 July 3, 2023 17:16
@greg7mdp
Copy link
Contributor Author

greg7mdp commented Jul 3, 2023

@heifner @arhag One concern I have is that this PR changes the serialization output for structs containing std::optional, when a value is omitted for one or more std::optional.
My first thought is that it may not be an issue, because the deserialization of these would have failed anyways, but I thought it would be worth mentioning.

@heifner
Copy link
Member

heifner commented Jul 3, 2023

@heifner @arhag One concern I have is that this PR changes the serialization output for structs containing std::optional, when a value is omitted for one or more std::optional. My first thought is that it may not be an issue, because the deserialization of these would have failed anyways, but I thought it would be worth mentioning.

I don't anticipate any issue here.

unittests/abi_tests.cpp Show resolved Hide resolved
}

{
// check conversion when the first optional members is missing
Copy link
Member

Choose a reason for hiding this comment

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

first should be second in the comment.

}

{
// check conversion when all optional members are provided
Copy link
Member

Choose a reason for hiding this comment

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

should be last.

unittests/abi_tests.cpp Outdated Show resolved Hide resolved
@greg7mdp greg7mdp merged commit c456d17 into release/3.2 Jul 5, 2023
16 checks passed
@greg7mdp greg7mdp deleted the gh-1189-3.2 branch July 5, 2023 15:14
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.

3 participants