-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add big int format to msgpack.pack #7811
base: main
Are you sure you want to change the base?
Conversation
shared-module/msgpack/__init__.c
Outdated
} else if ((int16_t)x == x) { | ||
write1(s, 0xd1); | ||
write2(s, x); | ||
STATIC void pack_int(msgpack_stream_t *s, mp_obj_t obj, bool _signed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity I would split this into 2 functions for the two cases:
STATIC void pack_small_int(msgpack_stream_t *s, mp_int_t value)
STATIC void pack_int(msgpack_stream_t *s, mp_obj_t obj)
@@ -195,19 +196,34 @@ STATIC mp_map_elem_t *dict_iter_next(mp_obj_dict_t *dict, size_t *cur) { | |||
return NULL; | |||
} | |||
|
|||
STATIC void pack_int(msgpack_stream_t *s, int32_t x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function works as is for pack_small_int
. I wouldn't bother trying to optimize for 1 byte of packing for some int values.
write1(s, 0xd1); | ||
write2(s, x); | ||
STATIC void pack_int(msgpack_stream_t *s, mp_obj_t obj, bool _signed) { | ||
byte buffer[9]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is already setup with write
functions to write its output. It seems unnecessary to add another layer of indirection (buffer
) to write the output.
@Neradoc This is still in progress, is that right? |
Yes I need to get back to it. |
This adds big ints to msgpack.pack, which can now encode 64-bit signed ints.
And now what we can encode and decode with msgpack should be on parity with C python.
Of note:
Differentiating big ints that could fit in 32 bits would require a bunch more code, and I don't even know how I would do that, so it's an optimization of encoding size for another day.
Fixes #6851
Fixes #7388
Some test code that encodes and decodes back to compare the result, catch errors, and also compare to the expected encoding based on C python (doesn't always match but that's super important IMO as long as it decodes the same).
Note that the OverflowError are marked as ✅ when expected.
Before:
After:
Python 3: