Suggestion: add additional compiler warnings

[martinbudden]

Any projects using the arduino-pico library are limited in setting their compiler warnings: they cannot set a compiler warning if arduino-pico generates that warning.

In my projects I like to turn warnings up to 11, but find that I am limited in what warnings I can set depending on what libraries I include.

arduino-pico is actually pretty good in that respect: it causes few addional warnings (-Wdouble-promotion was one exception).

I think the following compiler warnings could all be switched on:
arduino-pico does not fail with any of those warnings.
The reason for switching them on is so that the warnings were not inadvertently violated at some point in the future.

(Note that -Wall and -Wextra miss out quite a few warnings, hence the fairly long list).

-Werror
-Wall
-Wextra
-Wattributes
-Wdisabled-optimization
-Wdouble-promotion
-Wignored-qualifiers
-Wimplicit-fallthrough
-Wnarrowing
-Wsign-compare
-Wstrict-aliasing
-Wtrampolines
-Wunsafe-loop-optimizations
-Wunreachable-code
-Wunused-function

arduino-pico causes the following warnings in a few places, but it is a fairly small number of places and it would not take much work to be
able to switch on these warnings:

-Wmissing-declarations
-Wshadow
-Wunused-parameter 
-Wunused-value
-Wmissing-noreturn

The following warnings are violated by SD fat. I've made some PRs there to address -Wfloat-conversion, and it appears that -Wmissing-field-initializers is already being addressed.

-Wstrict-aliasing
-Wfloat-conversion
-Wmissing-field-initializers

[earlephilhower]

I'm not against adding additional warnings, but in my experience you hit diminishing returns rather quickly once you start going beyond Wall and Wextra.

You can do it yourself in a PR by editing tests/common.sh

arduino-pico/tests/common.sh

Lines 238 to 239 in b1787da

	echo "compiler.c.extra_flags=-Wall -Wextra -Werror -Wdouble-promotion -Wno-ignored-qualifiers $debug_flags" > rp2040/platform.local.txt
	echo "compiler.cpp.extra_flags=-Wall -Wextra -Werror -Wdouble-promotion -Wno-ignored-qualifiers -Wno-overloaded-virtual $debug_flags" >> rp2040/platform.local.txt

One of the bigger issues (and I hit this with the double promotion one) is that the Arduino API libs and examples (i.e. arduino.cc code) throw warnings as you crank up the dial. That extends to the other libs we pull in (like the awesome SDFat and Adafruit ones).

[martinbudden]

I'm not against adding additional warnings, but in my experience you hit diminishing returns rather quickly once you start going beyond Wall and Wextra.

There's the rub: any project is limited in the warings it can easily use by the lowest common denominator of all the libraries it includes.

One of the bigger issues (and I hit this with the double promotion one) is that the Arduino API libs and examples (i.e. arduino.cc code) throw warnings as you crank up the dial. That extends to the other libs we pull in (like the awesome SDFat and Adafruit ones).

Indeed. I've also made PRs to SDFat, pico-sdk, and arduino-esp32 it what is perhaps a forlorn attempt to raise this lowest common denominator. These PRs seem to be progressing those the review and approval stages so I think they will be accepted.

You can do it yourself in a PR by editing tests/common.sh

This is the sort of PR that is much more easily done by someone in the project, rather than someone outside the project. To do this properly I'd need to get to grips with any build variants you have and your test system etc. Whereas for someone within the project it could be done as a small increment to the work they do anyway. So I'll pass on that offer: I'll leave it to your judgement as to what (if any) additional warnings you enable.

I may revisit at some future date with some warning PRs if and when some of the libraries you pull in have made changes.

[earlephilhower]

I spent a few minutes tonight with these.

• -Wunused-parameter breaks in the SDK, which is why we add -Wno-unused-parameter to our project. When the SDK updates we will see if all those instances are fixed. One LWIP-wrapper unused param when in non-freertos noted, but a no-op.
• -Wunused-value doesn't seem to show anything in my quick tests, but a full CI would be needed to verify that
• -Wmissing-declaration seems to get confused with GCC aliases and GCC wrapped functions. A couple functions which could be static. But lots of noise. For wrapped functions it would be possible to make a header file from the wrapped definitions, but that header would never actually be used by GCC to check the call. The linker does the wrap renaming and doesn't know anything about the contents of the fcns it renames. You could define the wrapped function with different params from the real one, it'll never know until the code crashes on the chip.
• -Wmissing-noreturn flagged main1 which is fine, but expected.
• -Wshadow barfs on the Arduino API Interrupts.h and LittleFS. It's a technically correct warning, but there's no actual problem with the actual code that I can see (and it's been tested by pretty much every Arduino core, not just this one, and the LFS team). Did see one spot in the LFS error handling in the core proper if a directory/file creation happens.

I'll throw a PR for the minor changes to clear up some amount of those warnings, but for things like missing-declarations, I think that's just not going to happen because it's really just writing code to make the checker happy and not actually check anything.

[earlephilhower]

Arduino API fix: arduino/ArduinoCore-API#258

[earlephilhower]

LittleFS doesn't seem to be in agreement with adjusting code to fix -Wshadow. Discussion in an open PR ~3yrs ago: littlefs-project/littlefs#873 (and FWIW the mentioned code is still in there and still generating warnings)

[earlephilhower]

Closing this as not planned because the ones which are possible have been added and cleaned up with #3140. Others don't add value or spray too many non-useful messages IMHO. Thx for the push, anyway!