Enable implicit fallthrough diagnostic, note intentional fallthroughs. #3405
Conversation
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.
I investigated these cases and confirmed that the fallthrough behavior was intentional.
If any diagnostics occur, we will want to either add `/* FALLTHROUGH */` or `break;` as appropriate. I only tested a few builds (trinket m0 and metro m4 express)
jepler
force-pushed
the
implicit-fallthrough-diagnostic
branch
from
5b287b7 to
5729097
Compare
.. there is an instance of it that looks like a "true positive", but it only affects sdhc transfers that are not a multiple of 4 bytes, which I don't think happens. (sd card blocks are always 512 bytes) I can fix this in our asf4 repo but that would mean this should be deferred until after adafruit#3384 is merged, since that also touches asf4 very invasively by adding a whole new chip family.
|
For better or for worse, we apply more stringent warning flags to the atmel-samd port than to other ports. If you like, I can move it and we can find out how much more is caught with a wider net. |
|
I think it's a good idea, since it could catch a number of port-specific bugs. The main issue I had with the nRF port and its hal layers is that it does
I looked in |
|
Sorry for not being more specific, rather I have a general feeling of hitting some "wait, why didn't the compiler tell me" problems when not in the atmel sam port |
…diagnostic Conflict in locale/circuitpython.pot resolved with 'make translate'
|
cxd56 and mimxrt10xx don't enable -Werror. |
|
@dhalbert happy you're here, are these FALLTHROUGHs or missing break;s? |
Those are bugs! Both those |
|
Also you could remove the extraneous curly braces, which should only be there if there's a local declaration in the |
This is cumulative with #3404 since that also fixed a case of an unintentional fallthrough.
It's considered a common mistake to forget 'break' between cases of a case statement. Help the compiler help us find them in the first place.