-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
py/objint: Make byteorder argument optional in byte-conversion methods. #14387
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Amirreza Hamzavi <amirrezahamzavi2000@gmail.com>
Signed-off-by: Amirreza Hamzavi <amirrezahamzavi2000@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14387 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21204 21204
=======================================
Hits 20864 20864
Misses 340 340 ☔ View full report in Codecov by Sentry. |
Code size report:
|
Hi @AmirHmZz, Thanks for noticing this and submitting this PR.
To expand a bit on this, these That said, there are a number of improvements from later CPython versions that are supported in MicroPython and this one might be useful to have. 😁 I was going to suggest adding unit test cases for these as well, although it will be a little fiddly because the unit tests compare behaviour with CPython by default, and only some CPython versions will accept the optional argument. |
@projectgus One of the most important points about making that arg optional is reducing redundant allocations and deallocations on
Where should I add unit tests? I will be glad to contribute! |
I asked @dpgeorge about this change and it seems like it really will depend on the code size hit as to whether it's desirable to merge. It's currently small, which is encouraging. However, Damien noted that in Python 3.11 the
Thanks, that'd be great!
Suggest something like
I would have expected if you're either freezing your .py files into the firmware, or compiling to .mpy with mpy-cross, then the "big" string should end up interned as a QSTR with minimal performance impact. If you've got some "real world" type code where this change makes a significant performance difference, please share it as it'd be interesting to look into. |
Signed-off-by: Amirreza Hamzavi <amirrezahamzavi2000@gmail.com>
f78c099
to
f8961ea
Compare
Signed-off-by: Amirreza Hamzavi <amirrezahamzavi2000@gmail.com>
1bc8778
to
ca84705
Compare
@projectgus Thanks for feedback. I've added tests and also made length optional in |
Nice work @AmirHmZz, thanks! Implementation and tests look good to me. It's encouraging this implementation is free or almost free in terms of code size on most of the smaller microcontroller ports. |
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.
Looks good to me, I think whether this is merged will come down to @dpgeorge's call about the binary size cost.
One thing about the optional length argument for (Regrettably, that PR adds more than 8 bytes binary size! 😆) |
This PR intends to make
byteorder
argument optional inint.to_bytes()
andint.from_bytes()
methods. According to CPython documentation,byteorder
argument is optional and set to "big" by default. This PR makes Micropython compatible with CPython in this case.