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

Upgrade to msgpack-cpp 6.1.1 #146

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Upgrade to msgpack-cpp 6.1.1 #146

merged 2 commits into from
Aug 7, 2023

Conversation

StevenMaude
Copy link
Contributor

@StevenMaude StevenMaude commented Aug 7, 2023

Fixes #142.

This switches the vendored repository to a fork, for now. It should be reconsidered in future.

This is to work around the change of behaviour in msgpack-cpp in 4.1.2 as detailed in:

If floats and doubles have zero after a decimal point, msgpack-cpp in 4.1.2 and up packs them as ints.

Intend to replace this with a fork that reverts some changes for
compatibility with pdf2msgpack.
This is so we can incorporate changes needed for a new pdf2msgpack
release.

The branch used has changes made in msgpack-cpp 4.1.2 reverted.
This is because those changes result in floats and doubles getting
packed as ints when the value after the decimal is zero.
This breaks our current promise that values are floats.

Alternative fixes might be:

* change the specification, and downstream consumers have to convert
  ints to floats
* overload the packer to handle floats and doubles in the previous way

The current changes to msgpack-cpp are clean and do not require any other
changes beyond reverting the relevant commits. The net effect is that
the packing code for doubles and floats is exactly as it was in 4.1.1.
@StevenMaude
Copy link
Contributor Author

Tested this out with lots of inputs, and doesn't look any different to the v0.7.0 version, so merging.

@StevenMaude StevenMaude merged commit 4e18676 into master Aug 7, 2023
1 check passed
@StevenMaude StevenMaude deleted the msgpack-6.1.1 branch August 7, 2023 17:17
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.

Upgrade msgpack-cpp beyond v4.1.1
1 participant