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

object: add fixint support #1136

Closed

Conversation

joelguittet
Copy link

Because of custom parser on one side, I have to use fixint format so that positive/negative numbers are not truncated and keep the wanted size when packing an object.

I'm not sure this feature is wanted in the core library because probably this violate the philosophy of msgpack. I have an alternative solution which consist of re-implementing msgpack_pack_object, but I propose this anyway.

@redboltz
Copy link
Contributor

Thank you for sending the PR. I need some time to review it. Please wait a moment.

@redboltz
Copy link
Contributor

The specification states:

https://github.com/msgpack/msgpack/blob/master/spec.md#serialization

If an object can be represented in multiple possible output formats, serializers SHOULD use the format that represents the data in the smallest number of bytes.

Essentially, msgpack_object is designed to adhere to the msgpack specification. Therefore, I believe including FIXINT types within msgpack_object is not appropriate. This could also lead to issues with implementing symmetric unpacking.

From my review of your PR, it appears that your primary requirement is packing fixints. If you need to enforce a specific fixint size during packing, you can directly call the following functions in your code:

static int msgpack_pack_uint8(msgpack_packer* pk, uint8_t d);
static int msgpack_pack_uint16(msgpack_packer* pk, uint16_t d);
static int msgpack_pack_uint32(msgpack_packer* pk, uint32_t d);
static int msgpack_pack_uint64(msgpack_packer* pk, uint64_t d);
static int msgpack_pack_int8(msgpack_packer* pk, int8_t d);
static int msgpack_pack_int16(msgpack_packer* pk, int16_t d);
static int msgpack_pack_int32(msgpack_packer* pk, int32_t d);
static int msgpack_pack_int64(msgpack_packer* pk, int64_t d);
static int msgpack_pack_fix_uint8(msgpack_packer* pk, uint8_t d);
static int msgpack_pack_fix_uint16(msgpack_packer* pk, uint16_t d);
static int msgpack_pack_fix_uint32(msgpack_packer* pk, uint32_t d);
static int msgpack_pack_fix_uint64(msgpack_packer* pk, uint64_t d);
static int msgpack_pack_fix_int8(msgpack_packer* pk, int8_t d);
static int msgpack_pack_fix_int16(msgpack_packer* pk, int16_t d);
static int msgpack_pack_fix_int32(msgpack_packer* pk, int32_t d);
static int msgpack_pack_fix_int64(msgpack_packer* pk, int64_t d);

If you need to store type information somewhere (e.g., distinguishing between 0 as uint8, uint16, uint32), please consider handling this on the application side.

@joelguittet
Copy link
Author

Hello @redboltz
Thanks for the review I agree with you, this is what I was thinking when I said "I'm not sure this feature is wanted in the core library because probably this violate the philosophy of msgpack" in my initial message.
I already done the calls to the functions you mentioned in my specific application (communicating with a server I don't manage, this is why I need fixing).
I propose to close the pull request, if somebody is looking for similar topic, there is everything here to give a direction.
Joel

@redboltz
Copy link
Contributor

@joelguittet

Thank you for understanding. I believe this discussion will be valuable to others as well.

@redboltz redboltz closed this Aug 18, 2024
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