search

msgpack / msgpack-c

MessagePack implementation for C and C++ / msgpack.org[C/C++]
Other
3.03k stars 883 forks source link

object: add fixint support #1136

Closed joelguittet closed 3 months ago

joelguittet commented 3 months ago

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 commented 3 months ago

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

redboltz commented 3 months ago

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: https://github.com/msgpack/msgpack-c/blob/445880108a1d171f755ff6ac77e03fbebbb23729/include/msgpack/pack.h#L61-L77

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 commented 3 months ago

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 commented 3 months ago

@joelguittet

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