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

modstruct: Improve compliance with python3 #3404

Merged
merged 2 commits into from Sep 14, 2020

Conversation

jepler
Copy link
Member

@jepler jepler commented Sep 12, 2020

While checking whether we can enable -Wimplicit-fallthrough, I encountered a diagnostic in mp_binary_set_val_array_from_int which led to discovering the following bug:

>>> struct.pack("xb", 3)
b'\x03\x03'

That is, the next value (3) was used as the value of a padding byte, while standard Python always fills "x" bytes with zeros. I initially thought this had to do with the unintentional fallthrough, but it doesn't. Instead, this code would relate to an array.array with a typecode of padding ('x'), which is ALSO not desktop Python compliant:

>>> array.array('x', (1, 2, 3))
array('x', [1, 0, 0])

Possibly this is dead code that used to be shared between struct-setting and array-setting, but it no longer is.

I also discovered that the argument list length for struct.pack and struct.pack_into were not checked, and that the length of binary data passed to array.array was not checked to be a multiple of the element size.

I have corrected all of these to conform more closely to standard Python and revised some tests where necessary. Some tests for micropython-specific behavior that does not conform to standard Python and is not present in CircuitPython was deleted outright.

Testing performed: The unix port was built and the testsuite was run

While checking whether we can enable -Wimplicit-fallthrough, I encountered
a diagnostic in mp_binary_set_val_array_from_int which led to discovering
the following bug:
```
>>> struct.pack("xb", 3)
b'\x03\x03'
```
That is, the next value (3) was used as the value of a padding byte, while
standard Python always fills "x" bytes with zeros.  I initially thought
this had to do with the unintentional fallthrough, but it doesn't.
Instead, this code would relate to an array.array with a typecode of
padding ('x'), which is ALSO not desktop Python compliant:
```
>>> array.array('x', (1, 2, 3))
array('x', [1, 0, 0])
```
Possibly this is dead code that used to be shared between struct-setting
and array-setting, but it no longer is.

I also discovered that the argument list length for struct.pack
and struct.pack_into were not checked, and that the length of binary data
passed to array.array was not checked to be a multiple of the element
size.

I have corrected all of these to conform more closely to standard Python
and revised some tests where necessary.  Some tests for micropython-specific
behavior that does not conform to standard Python and is not present
in CircuitPython was deleted outright.
@jepler jepler merged commit 4d70872 into adafruit:main Sep 14, 2020
@tannewt
Copy link
Member

tannewt commented Sep 14, 2020

Who reviewed this? It's a good change. I'm just surprised to see it merged.

@dhalbert
Copy link
Collaborator

I think it got marked as merged automatically because its commit is part of the commits #3405, which I reviewed.

@jepler
Copy link
Member Author

jepler commented Sep 14, 2020

Right, the struct-pack-compliance branch was strictly a subset of #3405 so github marked this PR as merged when that one went in. I agree it's a surprise to see a PR merged with no reviews(!). Should I refrain from working like this, when a "smaller" issue spirals out of control and becomes a larger thing?

@tannewt
Copy link
Member

tannewt commented Sep 14, 2020

I'm totally OK to have larger PRs when it happens. I'd just suggest closing the smaller PR in that case.

@jepler jepler deleted the struct-pack-compliance branch November 3, 2021 21:10
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.

None yet

3 participants