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

Packed and unpacked types are not the same when using msgpack::type::variant. #1070

Closed
aint-no-programmer opened this issue May 13, 2023 · 5 comments

Comments

@aint-no-programmer
Copy link

Problem appears when using msgpack::type::variant serialization for float number with zero decimal part.
Here motivation example:

# include <msgpack.hpp>
# include <cassert>

int main() {
	msgpack::type::variant val(212.f);
	std::stringstream ss;
	msgpack::pack(ss, val);

	auto unpacked = msgpack::unpack(ss.str().data(), ss.str().size());
	auto obj = unpacked.get();
	msgpack::type::variant restored_val;
	obj.convert(restored_val);

	assert(val == restored_val);//failed here
}

The reason for the failed assert is different types of variant’s values.
There are float type of val and uint64 type of the restored_val
I’ve found out that positive float number with zero decimal part is stored as uint64, hence when it is deserialized, it turns into an uint64 number.

As I know, it happens in line 1165 of msgpack/v1/pack.hpp, here:

template <typename Stream>
inline packer<Stream>& packer<Stream>::pack_double(double d)
{
    if(d == d) { // check for nan
        // compare d to limits to avoid undefined behaviour
        if(d >= 0 && d <= double(std::numeric_limits<uint64_t>::max()) && d == double(uint64_t(d))) {
            pack_imp_uint64(uint64_t(d));
            return *this;
        } else if(d < 0 && d >= double(std::numeric_limits<int64_t>::min()) && d == double(int64_t(d))) {
            pack_imp_int64(int64_t(d));
            return *this;
        }
    }
    //other code

Here https://gist.github.com/redboltz/672c5af16b2907488977 same problem.

Thank you for response.

msgpack version: 6.0.0
OS: Windows 10

@redboltz
Copy link
Contributor

Thank you for reporting the issue. I think that it is caused by #1018 (#1017).
If after the decimal point is 0 then use INT format family.
This is allowed and expected behavir (See #1017 discusstion).

I will take a look msgpack::type::variant implementation and will try to fix msgpack::type::variant side.

redboltz added a commit to redboltz/msgpack-c that referenced this issue May 14, 2023
- msgpack::type::variant behaves as MessagePack format.
  e.g.)
  12.34  => double
  12.0   => uint64_t
  -12.34 => double
  -12.0  => int64_t
- msgpack::type::variant::as_double() can be used even if interval type is
  int64_t and/or uint64_t.
- msgpack::type::variant::as_*() don't return non const reference
  internal value.
- fix coding style
@redboltz
Copy link
Contributor

Fixed. Could you try #1071 ?

@aint-no-programmer
Copy link
Author

Yes, i tried it works.
But now i have uint64 type stored in msgpack::type::variant, when i know i put the float type there.
Now it turns out that the type of the placed value does not match the type of the stored value.
Isn't that a problem?

And second, if i want to use user defined variant as in your example here i will face this problem again.

@redboltz
Copy link
Contributor

redboltz commented May 14, 2023

Yes, i tried it works.
But now i have uint64 type stored in msgpack::type::variant, when i know i put the float type there.
Now it turns out that th

No, it is NOT a problem.

MessagePack format doesn't preserve progiramming language type information. That is by design.
msgpack::type::variant should be corresponding to MessagePack format, not the programming language type.
So msgpack::type::variant doesn't preserve the original type. That is by design. But you can extract as double from int64_t or uint64_t.

And second, if i want to use user defined variant as in [your example here]

I don't maintain old my gist. User can define type preserve version of custom adaptor if user wants to have it. In order to do that, the user need to pack not only values but also meta data (type information).

@aint-no-programmer
Copy link
Author

I got it. Thank you for clarification.

redboltz added a commit that referenced this issue May 15, 2023
StevenMaude added a commit to sensiblecodeio/msgpack-c that referenced this issue Aug 7, 2023
This reverts commit 34f8fd6.
StevenMaude added a commit to sensiblecodeio/msgpack-c that referenced this issue Feb 5, 2024
This reverts commit 34f8fd6.
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

No branches or pull requests

2 participants