raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.68k stars 917 forks source link

Mark headers as SYSTEM includes in CMake #82

Closed mattgodbolt closed 1 year ago

mattgodbolt commented 3 years ago

The include paths are set using:

    target_include_directories(pico_stdlib_headers INTERFACE include)

(e.g. in src/common/pico_stdlib/CMakeLists.txt)

As they're not marked as SYSTEM, enabling warnings (e.g. -Wall) will erroneously be tagged in the system headers.

pico-sdk/src/rp2040/hardware_regs/include/hardware/regs/addressmap.h:56:20: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
   56 | #define TIMER_BASE 0x40054000

for example. If the headers are tagged SYSTEM then CMake uses -Isystem or equivalent which tells the compiler to suppress warnings coming from those includes.

A potential fix is:

$ git grep -l target_include_directories | xargs perl -pi -e 's/target_include_directories(.*)INTERFACE/target_include_directories$1SYSTEM INTERFACE/'
mattgodbolt commented 3 years ago

Modifying a simple example with target_compile_options(test PRIVATE -Wall -Wextra -Werror) shows up the improvement; though the numerous built-on-demand files (e.g. /home/mgodbolt/dev/personal/pico/pico-sdk/src/rp2_common/hardware_sync/sync.c:25) get built with these flags too and fail to compile. My cmake-fu is a bit weak at this point, as I can't see how test's PRIVATE flags are affecting things over in hardware_sync/sync.c but maybe someone stronger in this area can help.

kilograham commented 3 years ago

If you look at test/kitchen_sink in the SDK, that builds everything with higher warning levels (and werror) than we do by default. That is the level of cleanliness we currently strive for.

This will get built if you build from pico-sdk directly, or you pass -DPICO_SDK_TESTS_ENABLED=1 to your build elsewhere.

Have to have a think about making these system.

mattgodbolt commented 3 years ago

Got it: thanks. I don't think I quite realised the SDK builds as part of a regular build (which is both ace but also means some compiler settings get picked up by it). Great job though; It Just Works™ :)

kilograham commented 3 years ago

possibly worth seeing the state of things now (from a warnings perspective) and see if that helps any. (in the develop branch which is the latest)

kilograham commented 1 year ago

close in favor of #954