Skip to content

Check parameters of int.from_bytes #6562

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 19, 2022
Merged

Check parameters of int.from_bytes #6562

merged 8 commits into from
Jul 19, 2022

Conversation

isacben
Copy link

@isacben isacben commented Jul 6, 2022 •
edited
Loading

Hi,

I am opening this PR to solve #6419. This is what I am trying to do:

  1. Allow the method to still receive 3 parameters, since signed parameter support still needs to be implemented.
  2. Add support to accept the signed keyword parameter; I did not implement signed parameter support though, but added a NotImplementedError if signed=True is passed.
  3. Added a validation for the byteorder parameter. Not sure if this is was necessary. I added MP_QSTR_big here, but I am not sure how to the qstrdefs.enum.h file.
  4. Tested with adafruit_feather_rp2040.

Please let me know if you have any feedback so I can review.

Thank you!

dhalbert
dhalbert requested changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I have made some suggestions (which are not tested!).

Added a validation for the byteorder parameter. Not sure if this is was necessary. I added MP_QSTR_big here, but I am not sure how to the qstrdefs.enum.h file.

The qstr will get added automatically; you don't need to do anything. The sources are scanned to generate the qstr's when a build is done. Make sure to do make clean if you change or add any qstr's, though.

I don't know whether you changed circuitpython.pot by hand or not. You don't need to. Just run make translate before committing, and it will be updated automatically.

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 6, 2022

The Metro M0 Express ru build is too big. After you make some changes, we'll see if we can squeeze it a bit.

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 6, 2022

@isacben I just want to know if you retested.

@isacben
Copy link
Author

isacben commented Jul 6, 2022

Hi @dhalbert ,

Thank you very much for the helpful feedback!!

Yes, I am testing with the Feather RP2040. This is the only one I have, unfortunately!

  • I wend ahead and made the changes (also tested with Feather RP2040).
  • Regarding the circuitpython.pot, I did not updated manually; the pre-commit did it. This time I did run make translate.
  • I also realized about the MO Express. I built CP for that board locally and it worked; so, I would like to see how it behaves with this changes in the CI environment.
  • This second commit failed. I think it was because of this line:
{ MP_QSTR_bytes, MP_ARG_REQUIRED | MP_ARG_OBJ },
{ MP_QSTR_byteorder, MP_ARG_REQUIRED | MP_ARG_OBJ },

I think this would fix it:

{ MP_QSTR_bytes, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
{ MP_QSTR_byteorder, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },

Please let me know what you think.

Thank you in advance!!

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 6, 2022 •
edited
Loading

  • This second commit failed. I think it was because of this line:
{ MP_QSTR_bytes, MP_ARG_REQUIRED | MP_ARG_OBJ },
{ MP_QSTR_byteorder, MP_ARG_REQUIRED | MP_ARG_OBJ },

I think this would fix it:

{ MP_QSTR_bytes, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
{ MP_QSTR_byteorder, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },

This is only because the unix port has set -Wextra. A cheaper way would be:

        { MP_QSTR_bytes, MP_ARG_REQUIRED | MP_ARG_OBJ, {0} },
        { MP_QSTR_byteorder, MP_ARG_REQUIRED | MP_ARG_OBJ, {0} },

@isacben
Copy link
Author

isacben commented Jul 6, 2022

Thanks Dan! I will take your advice on the initialization.

It seems now the build is too big for the Metro MO Express and the Nano 33 IoT. Please let me know if there is any recommendation on this regard. Probably I went too far with the idea of adding the signed parameter and the byteorder validation!

Thank you!

@dhalbert
Copy link
Collaborator

dhalbert commented Jul 8, 2022

@isacben I will not be able to work on this for about a week or so, but will get back to you about shrinking those builds. Thanks for working on this.

@isacben
Copy link
Author

isacben commented Jul 8, 2022

Hi @dhalbert,
No worries! Thanks for the heads up. I will be around.
Thank you.

dhalbert
dhalbert approved these changes Jul 19, 2022
View reviewed changes
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes to make things fit, and updated some infrastructure things.

@jepler @tannewt I made the unix port use -std=gnu11 instead of gnu99. We don't need to support really old compilers. Also made a change to not include the ru font on SAMD21 builds, and built Feather ESP32-S3 TFT with -Os instead of -O2, rather than starting to disable more modules to make it fit.

@dhalbert dhalbert merged commit 9d95fc1 into adafruit:main Jul 19, 2022
@isacben
Copy link
Author

isacben commented Jul 20, 2022

Hi @dhalbert,

Thank you very much for your help! I will have a look to the changes you made to learn about ways to shrink the builds!

Thank you!

@isacben isacben deleted the from_bytes-check-parameters branch July 23, 2022 22:14
@skerr92 skerr92 mentioned this pull request Oct 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@isacben @dhalbert