Make: enable -Werror -Wall -Wextra by default#7919

Merged

cladmi merged 2 commits intoRIOT-OS:masterfrom

smlng:make/wpedantic

Nov 28, 2017

Member

smlng commented Oct 31, 2017

This picks up the idea of #1121 (which isn't mergeable anymore, because of considerable changes since then) to enable more compiler warnings and make them errors to fail. This PR also ships several fixes to make things work.

It also provides an option to turn on -Wpedantic too, however, that would require even more work and also some agreement (e.g., usage of gcc extensions, see #7918).

may need #7860 and other existing PR that solve open issues.

smlng added Type: cleanup Type: enhancement Area: build system labels

smlng requested review from OlegHahm, haukepetersen, jnohlgard and kaspar030

October 31, 2017 18:41

smlng added State: WIP CI: ready for build labels

smlng mentioned this pull request

make: buildtest with "-Wall -Wextra -Werror" #1121

Closed

Member Author

smlng commented Oct 31, 2017

also tracked issues in #7875 need to be fixed for this.

miri64 assigned jnohlgard, haukepetersen, OlegHahm and kaspar030

basilfx mentioned this pull request

test: periph_timer: check if argument is passed. #7923

Merged

Contributor

kaspar030 commented Nov 1, 2017

While most of the fix commits are trivial, some are not, which will make reviewing difficult.

I suggest you make this PR only enable the extra warnings, and then open smaller PR's fixing the actual compile issues. You could group them (e.g., all "unused parameter" fixes, then all "signed compare" ...).

Member Author

smlng commented Nov 1, 2017

@kaspar030 you're right, on first sight I thought: just a few fixes, make it one PR. But then the list got longer and longer and longer ... as always, will split in groups as suggested.

cladmi suggested changes

View reviewed changes

Contributor

cladmi left a comment

I just did a read through code review, not tested.

Could be a good idea to add a "RM ME later" commit that makes WPEDANTIC by default to see murdock output.

cpu/cc2538/include/cpu_conf.h Outdated

               #include "cpu_conf_common.h"
+              /* store compiler switches */
+              #pragma GCC diagnostic push
+              #pragma GCC diagnostic ignored "-Wpedantic"

Contributor

cladmi Nov 9, 2017

For this warning ignoring, I would prefer to see why its disabled than "store compiler switches".

Member Author

smlng Nov 9, 2017

yes right, here and below its mostly to exclude vendor headers from being checked with pedantic on - because we don't want to change or fix them but rather use the originals and also update them from time to time

cpu/cc26x0/include/cpu_conf.h Outdated

+              /* store compiler switches */
+              #pragma GCC diagnostic push
+              #pragma GCC diagnostic ignored "-Wpedantic"

Contributor

cladmi Nov 9, 2017

Same here, comment why pedantic is disabled.

cpu/k22f/include/cpu_conf.h Outdated

+              /* store compiler switches */
+              #pragma GCC diagnostic push
+              #pragma GCC diagnostic ignored "-Wpedantic"

Contributor

cladmi Nov 9, 2017

Same here, comment why pedantic is disabled.

cpu/k60/include/cpu_conf.h Outdated

               #ifndef CPU_CONF_H
               #define CPU_CONF_H
+              /* store compiler switches */

Contributor

cladmi Nov 9, 2017

Same here, comment why pedantic is disabled.

cpu/k64f/include/cpu_conf.h Outdated

               #ifndef CPU_CONF_H
               #define CPU_CONF_H
+              /* store compiler switches */

Contributor

cladmi Nov 9, 2017

Same here, comment why pedantic is disabled.

cpu/kw2xd/include/cpu_conf.h Outdated

               #ifndef CPU_CONF_H
               #define CPU_CONF_H
+              /* store compiler switches */

Contributor

cladmi Nov 9, 2017

Same here, comment why pedantic is disabled.

cpu/saml21/periph/timer.c

                */
               int timer_init(tim_t dev, unsigned long freq, timer_cb_t cb, void *arg)
               {
+                  /* at the moment, the timer can only run at 1MHz */

Contributor

cladmi Nov 9, 2017

This is not related to -Wall -Wextra, or is it ?

Member

miri64 Nov 28, 2017

This is not related to -Wall -Wextra, or is it ?

Ping @smlng?

Member Author

smlng Nov 28, 2017

mhm, IIRC copy pasted from samd21/periph/timer.c, but don't know why/how it slipped in here. Shall I remove?

Contributor

cladmi Nov 28, 2017

I don't care it can stay.

drivers/at86rf2xx/at86rf2xx_getset.c

-                  if ((channel < AT86RF2XX_MIN_CHANNEL) ||
-                      (channel > AT86RF2XX_MAX_CHANNEL) ||
+                  if ((channel > AT86RF2XX_MAX_CHANNEL) ||
+              #if AT86RF2XX_MIN_CHANNEL /* is zero for sub-GHz */

Contributor

cladmi Nov 9, 2017

Maybe duplicate the if lines for readability here, like in _netdev.c

Member

miri64 Nov 28, 2017

Maybe duplicate the if lines for readability here, like in _netdev.c

Ping @smlng?

Member Author

smlng Nov 28, 2017

well, maybe not?

Contributor

cladmi Nov 28, 2017

Same here, its just style.

smlng force-pushed the make/wpedantic branch from fb79a83 to 0ec65b2 Compare

November 10, 2017 08:58

This was referenced Nov 10, 2017

make: use -isystem for CPU vendor header includes #7993

Closed

make: fix sign-compare errors #7994

Merged

make: fix unused-params error #7995

Merged

Member Author

smlng commented Nov 10, 2017

separated into fixes into groups as suggested by @kaspar030, this PR now requires #7993, #7994, #7995, and other before merge.

smlng added the State: waiting for other PR label

smlng force-pushed the make/wpedantic branch from 0ec65b2 to 7539e0f Compare

November 10, 2017 09:08

Member Author

smlng commented Nov 10, 2017

rebased onto #7993, #7994, #7995 -> they are now included here, to check with Murdock whats still missing,

Contributor

cladmi commented Nov 10, 2017

Please let us know when you finished the sub-PRs so we can review them.

Member Author

smlng commented Nov 10, 2017 •

edited

Loading

@cladmi yep, will do - forgot to mark it them WIP, sorry

smlng force-pushed the make/wpedantic branch from 7539e0f to ae063ea Compare

November 10, 2017 10:53

Member Author

smlng commented Nov 28, 2017

we also could do that in a separate PR.

sure, though I like all to have all green 😉

Member

miri64 commented Nov 28, 2017

When this get's merged today, you should write a reminder to the maintainers (and maybe even devel) mailing list that all builds on Murdock should be rebuild before merging ;-). Otherwise older PRs could re-introduce new bugs.

Member Author

smlng commented Nov 28, 2017

murdock looks good, may I squash?

Member

miri64 commented Nov 28, 2017

Yes, please!

smlng force-pushed the make/wpedantic branch from d091574 to 462373a Compare

November 28, 2017 15:18

Member Author

smlng commented Nov 28, 2017

squashed

Member Author

smlng commented Nov 28, 2017

@cladmi can you have another look, so we can merge this soonish! Merci!

Member Author

smlng commented Nov 28, 2017

ping! This might be worth two (or more) ACKs before merge?!

smlng added the Community: Hack'n'ACK candidate label

smlng mentioned this pull request

net, lwmac: cleanup, restructured code #8167

Merged

miri64 approved these changes

View reviewed changes

Member

miri64 left a comment

Ok. I'm fine with this.

Member

miri64 commented Nov 28, 2017

(The rebuild rule should be applied here as well, before hitting the green button, btw ;-))


          make: fix various compile errors with Wextra

b283b77

        pkg, nordic_softdevice_ble: disable CFLAGS to omit compiler error
        sys, pm_layered: fix casting nonscalar to the same type
        cpu, stm32_common: fix type-limits, remove always true assert
        cpu, stm32f4: fix pointer arithmetic in periph/i2c
        drivers, at86rf2xx: fix type-limits where condition always true
        saul, gpio: fix if no gpio configured for saul
        cpu, saml21: add frequency check to periph/timer
        driver, cc110x: fix unused param and type-limts errors
        boards, wsn430-common: fix old-style-declaration
        make: fix old style definition
        drivers, sdcard_spi: fix old style typedef
        driver, at30tse: remove unnecessary check
        driver, nrf24: fix type-limit
        driver, pn532: change buffer from char to uint8_t
        tests/driver_sdcard: fix type limits
        boards, feather-m0: add missing field inits
        driver, tcs37727: fix type limits
        pkg, emb6: disable some compiler warnings
        tests/emb6: disable some compiler warings
        pkg, openthread: fix sign compare and unused params
        tests/trickle: fix struct init
        tests/pthread_cooperation: fix type limits
        board, mips-malta: remove feature periph_uart
        shell: fix var size for netif command
        gnrc, netif: fix sign-compare
        gnrc, nib: fix sign-compare
        shell: fix output in netif command
        posix: fix type-limits in pthread_cond

smlng force-pushed the make/wpedantic branch from 462373a to cbc23b5 Compare

November 28, 2017 17:32

cladmi suggested changes

View reviewed changes

drivers/nrf24l01p/nrf24l01p.c Outdated

                   }
-                  if (width < 0) {
+                  if (width == 0) {

Contributor

cladmi Nov 28, 2017

width == 0 was not an error before.

Member Author

smlng Nov 28, 2017

? width < 0 also matches width == 0 and as width is unsigned its the same

Member

miri64 Nov 28, 2017

No... width < 0 is not equivalent to width == 0, even if unsigned... An unsigned can be 0, and width < 0 would still be false.

Member Author

smlng Nov 28, 2017

you're right, its not the same. However, width is unsigned so < 0 makes no sense, further it also makes no sense to set payload length 0, too. So the check is still valid (IMHO).

Member

miri64 Nov 28, 2017

But it changes the meaning completely now (let's not even talk about the API change). If 0 wasn't an error case before, you made it one now.

Member Author

smlng Nov 28, 2017

well then the checked can be removed completely, if that's the (intermediate) solution - leaving a side that width = 0 should be wrong anyway.

Member

miri64 Nov 28, 2017

Since this PR is not about changing any functional errors I would prefer the intermediate solution and a fix as a follow-up.

Member Author

smlng Nov 28, 2017

okay

cpu/saml21/periph/timer.c

                */
               int timer_init(tim_t dev, unsigned long freq, timer_cb_t cb, void *arg)
               {
+                  /* at the moment, the timer can only run at 1MHz */

Contributor

cladmi Nov 28, 2017

I don't care it can stay.

drivers/at86rf2xx/at86rf2xx_getset.c

-                  if ((channel < AT86RF2XX_MIN_CHANNEL) ||
-                      (channel > AT86RF2XX_MAX_CHANNEL) ||
+                  if ((channel > AT86RF2XX_MAX_CHANNEL) ||
+              #if AT86RF2XX_MIN_CHANNEL /* is zero for sub-GHz */

Contributor

cladmi Nov 28, 2017

Same here, its just style.

tests/trickle/main.c Outdated

-              static trickle_t trickle = { .callback.func = &callback,
-                                           .callback.args = NULL };
+              static trickle_t trickle;

Contributor

cladmi Nov 28, 2017

What was the problem with this one ?

Member Author

smlng Nov 28, 2017

the way of init the struct caused an error.

Member Author

smlng Nov 28, 2017

so separated definition and init

Member

miri64 Nov 28, 2017

maybe use

static trickle_t trickle = { .callback = { .func = &callback,
                                           .args = NULL } };

instead? It doesn't change the behavior and is the more correct version.

Contributor

cladmi Nov 28, 2017

For native, iotlab-m3, samr21-xpro I get no error with the first version.

What was the error ? the commit message does not help me "tests/trickle: fix struct init"

Member Author

smlng Nov 28, 2017

Error was reported by clang on macOS, which is often more pedantic than gcc

Member Author

smlng Nov 28, 2017

mhm, can't reproduce right now - maybe it was reported by Murdock on some other platform (too). At least the old init version didn't work, @miri64 solution looks okay to me, too.

cladmi approved these changes

View reviewed changes

Contributor

cladmi commented Nov 28, 2017

Only need to squash and wait for murdock.


          make: enable Wall and Wextra by default, and optionally Wpedantic

57c5c9b

smlng force-pushed the make/wpedantic branch from 58d167c to 57c5c9b Compare

November 28, 2017 19:33

Member Author

smlng commented Nov 28, 2017

squashed

cladmi merged commit b54318c into RIOT-OS:master

miri64 mentioned this pull request

pkg: ccn-lite: adapt to v2 #7437

Merged

kYc0o mentioned this pull request

drivers/sdcard_spi #7853

Closed

smlng mentioned this pull request

cpu, efm32_common: fix sign-compare error in flashpage #8175

Merged

pyropeter added a commit to beduino-project/RIOT that referenced this pull request


          drivers/kw2xrf: fix fall through

19f6544

This was made visible by RIOT-OS#7919. Broke build on GCC 7.2.

Probably a bug.

pyropeter mentioned this pull request

drivers/kw2xrf: fix fall through #8192

Merged

aabadie added this to the Release 2018.01 milestone

smlng deleted the make/wpedantic branch

February 6, 2018 19:42

aabadie mentioned this pull request

"Minor" compiling issues found by clang #7875

Closed

16 tasks

panail pushed a commit to panail/RIOT that referenced this pull request


          drivers/kw2xrf: fix fall through

8345a4f

This was made visible by RIOT-OS#7919. Broke build on GCC 7.2.

Probably a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

miri64 miri64 approved these changes

jnohlgard Awaiting requested review from jnohlgard

haukepetersen Awaiting requested review from haukepetersen

OlegHahm Awaiting requested review from OlegHahm

kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

+1 more reviewer

cladmi cladmi approved these changes

Labels

Area: build system CI: ready for build Community: Hack'n'ACK candidate Type: cleanup Type: enhancement