fix(QPG): Enable -Wundef and fix any errors for qpg based builds #29617
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov and
lpbeliveau-silabs
pullapprove
bot
requested review from
plauric,
rcasallas-silabs,
saurabhst,
selissia,
sharadb-amazon,
tecimovic,
tehampson,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #29617: Size comparison from 2d07e21 to 75afa04 Full report (1 build for cc32xx)
|
Damian-Nordic
approved these changes
Oct 6, 2023
|
PR #29617: Size comparison from 2d07e21 to ecabdb6 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Oct 6, 2023
| if (chip_enable_pw_rpc) { | ||
| defines = [ "PW_RPC_ENABLED" ] | ||
| } | ||
| defines = [ "PW_RPC_ENABLED=${chip_enable_pw_rpc}" ] |
There was a problem hiding this comment.
I guess the semantics of this macro (boolean value vs just-defined-or-not) are per-platform, so it's OK that this is not necessarily going to be consistent with other platforms?
| * @brief Defines if a matter device is acting as a Synchronized Sleepy End Device(SSED) | ||
| */ | ||
| #ifndef CHIP_DEVICE_CONFIG_ENABLE_SSED | ||
| #define CHIP_DEVICE_CONFIG_ENABLE_SSED 0 |
There was a problem hiding this comment.
This is fine, but really this should have gone in CHIPDeviceConfig.h (the core one)... I suspect it will get added there as part of #29613
| @@ -184,7 +184,7 @@ CHIP_ERROR CHIP_Init(void) | |||
| goto exit; | |||
| } | |||
|
|
|||
| #if CONFIG_CHIP_THREAD_SSED | |||
| #if defined(CHIP_DEVICE_CONFIG_ENABLE_SSED) && CHIP_DEVICE_CONFIG_ENABLE_SSED | |||
There was a problem hiding this comment.
This on the other hand looks wrong. There is no CHIP_DEVICE_CONFIG_ENABLE_SSED so far (before this PR). What we do have is CHIP_DEVICE_CONFIG_THREAD_SSED on some platforms and CONFIG_CHIP_THREAD_SSED on some platforms, and it's a bit of a mess.
But generally:
- All
CHIP_*CONFIG*macros must have a specific config header responsible for ensuring they are defined (and pulling in all the headers that might define them). - Consumers of such macros should test them with
#ifand include the responsible header.
The setup here, where defined is tested, makes it too easy to fail to include the header that actually defines the config and then build different parts of the code with incompatible configurations...
I know the config header setup is really weird; I am only now starting to finally understand how it works. But the above is how it should work. ;)
| @@ -1710,7 +1710,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::DoInit(otInstanc | |||
| VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); | |||
|
|
|||
| // Enable automatic assignment of Thread advertised addresses. | |||
| #if OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE | |||
| #if defined(OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE) && OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE | |||
There was a problem hiding this comment.
Again, ideally there would be a specific config header responsible for this that we ensured we included. In practice people put this define in all sorts of random places: project-specific OpenThreadConfig.h, platform-specific CHIPDevicePlatformConfig.h, some openthread-core-cc13x2_26x2-config.h and bl702-openthread-core-bl-config.h headers, etc, etc. I have zero confidence this file is ending up including those headers. For example, lots of places are setting openthread_project_core_config_file but I don't see that being used anywhere.
So as far as I can tell, this change is just covering up a bug: we are not including the things that might define OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE in this file.
There was a problem hiding this comment.
Follow up created - #29624
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…ject-chip#29617) * fix(QPG): Enable -Wundef and fix any errors for qpg based builds (Addresses project-chip#29591) * Restyled by gn --------- Co-authored-by: Restyled.io <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#29591 uncovered errors raised by -Wundef.
This PR addresses all issues found by -Wundef.
Note -Wundef is not enabled by default yet in all builds - flag added to qpg specific gn configuration.
Tested: