[Build] Stricter compiler warning settings

[rojer]

Build issue(s)

It's not a bug as such, rather a suggestion/discussion. I'm a relative newcomer to the project, currently busy integrating CHIP/Matter support into our product line. In our environment we build with stricter compiler warnings, so I had to adapt.
In my opinion, some of them might be useful tot he project (and as an added benefit to us, we won't have to suppress warnings and patch sources). We compile with -Wextra, but specifically I am talking about:

1. -Wundef - I find it extremely useful. I actually got most of the CHIP to compile with -Wundef with only a minor change (Convert if __ZEPHYR__ to if defined(__ZEPHYR__) #29172). It helped me a lot during porting, since it exposed the macros I had to define, instead of just assuming false value. I think any project would benefit from -Wundef. For example, it exposed a minor bug.
2. -Wunused-parameter - I find it helpful to have compiler remind that an argument passed to a function is not used. Sometimes it leads to reconsidering the function signature to get rid of the paramter, and in legitimate cases it can be be easily cast to void or annotated with __attribute__((unused)) (usually via a macro).

My proposal is thus to use -Wextra, at least for core components (as of today it already works, at least with clang-15), or at least add -Wundef which seems to be clearly useful. -Wunused-parameter is debatable but also a nice-to-have, in my opinion.

Platform

all

Anything else?

No response

[bzbarsky-apple]

-Wextra is enabled by default in our CI. See

connectedhomeip/build/config/compiler/BUILD.gn

Line 236 in b02c984

"-Wextra",

It looks like -Wextra does not imply -Wundef, at least per https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-W. Turning that on does seem useful.

-Wextra does imply -Wunused-parameter if we are also using -Wall, as we are, but then it's explicitly turned off at

connectedhomeip/build/config/compiler/BUILD.gn

Line 199 in b02c984

"-Wno-unused-parameter",

. Looks like there are a lot of failures for this one, especially for methods that are returning "not implemented" or are otherwise optional-to-do-anything-in stubs. Annotating all the arguments to those one at a time as unused would be a pain; I suspect that to enable -Wunused-parameter we would want some way to annotate "this method uses none of its parameters".

[bzbarsky-apple]

I just tried enabling -Wundef and it fails with all sort of errors. Some of them seem pretty straightforward to fix, but a few look complicated:

• BIGENDIAN_CPU is going to be a bit of a pain. See also BIGENDIAN_CPU is not defined in the build system for big-endian builds #7281
• LWIP_PBUF_FROM_CUSTOM_POOLS just looks broken. Filed [BUG] LWIP_PBUF_FROM_CUSTOM_POOLS and PBUF_POOL_SIZE don't seem to be set up in a reasonable way #29211
• CHIP_CONFIG_ENABLE_READ_CLIENT also looks broken. Filed [BUG] Configuration for CHIP_CONFIG_ENABLE_READ_CLIENT looks broken #29213
• Similar with MATTTER_TRACING_ENABLED. Filed [BUG] Configuration for MATTTER_TRACING_ENABLED looks broken #29214
• And CHIP_MINMDNS_HIGH_VERBOSITY. Filed [BUG] Configuration for CHIP_MINMDNS_HIGH_VERBOSITY looks broken #29215
• CHIP_CONFIG_NON_POSIX_LONG_OPT is not broken, but fails with -Wundef. Filed [BUG] CHIP_CONFIG_NON_POSIX_LONG_OPT needs to be defined when false #29216
• ICD_REPORT_ON_ENTER_ACTIVE_MODE is kind of broken. Filed [BUG] Need to have an actual way of defining ICD_REPORT_ON_ENTER_ACTIVE_MODE #29217

[rojer]

@bzbarsky-apple wow, thanks for the detailed response! you are correct, -Wundef is not part of -Wextra, we add it explicitly (i did it way back when and have since forgotten about it). our full warning settings are -Wall -Wextra -Wundef -Werror, i find it a nice combo.
i see you are well on the way to enabling -Wundef - this is great, as i said, i found it invaluable in general and in porting CHIP in particular.
WRT -Wno-unused-parameter - i am not aware of a way to disable it for an entire function, unfortunately. a reasonably brief macro, say CHIP_UNUSED, should do the trick and not produce too much clutter. and it if it gets repetitive and most uses don't use most args - maybe it's a sign that API should be reconsidered.
but this is no big deal if it doesn't get adopted. i'm happy that -Wundef will.
thanks again!