Add -Werror to acceptance builds for C and CPP #4369

earlephilhower · 2018-02-15T04:35:36Z

-edit- This is now completed and there are no errors or warnings reported when run with -Wall -Werror as part of the build process. Ready for review

Use platform.local.txt to add -Werror to GCC for the build of all
code. Any warnings on a submitted patch will cause an error.

earlephilhower · 2018-02-15T05:16:08Z

It caught the warning as an error. Will remove the OTA test change and put in a final git commit:

...
/sketch/BasicOTA.ino.cpp.o"
/home/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino: In function 'void loop()':
/home/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino:64:5: error: unused variable 'x' [-Werror=unused-variable]
 int x = 0;
     ^
cc1plus: all warnings being treated as errors
Using library ESP8266WiFi at version 1.0 in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266WiFi 
Using library ESP8266mDNS in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266mDNS (legacy)
Using library ArduinoOTA at version 1.0 in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ArduinoOTA 
exit status 1
Sketch:  /home/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino
Build dir:  /home/travis/build/esp8266/Arduino/build.tmp
Output:  /home/travis/build/esp8266/Arduino/build.tmp/BasicOTA.ino.bin
Building: /home/travis/arduino_ide/arduino-builder -compile -logger=human -build-path "/home/travis/build/esp8266/Arduino/build.tmp" -tools "/home/travis/arduino_ide/tools-builder" -libraries "/home/travis/Arduino/libraries" -hardware "/home/travis/arduino_ide/hardware" -hardware "/home/travis/build/esp8266/Arduino/tools/../cores" -fqbn=esp8266com:esp8266:generic:CpuFrequency=80,FlashFreq=40,FlashMode=qio,UploadSpeed=921600,FlashSize=512K64,ResetMethod=nodemcu -ide-version=10607 -warnings=all -verbose /home/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino
The command "$TRAVIS_BUILD_DIR/tests/common.sh" exited with 1.

earlephilhower · 2018-02-15T16:29:17Z

Ugh, looks like many examples and even several libraries have problems when -Wall is enabled. This is not as straightforward as I'd hoped.

d-a-v · 2018-02-16T09:35:14Z

I very much like the idea of -Wall for the tests. Should we be using -Wextra too ?

earlephilhower · 2018-02-16T17:18:26Z

At some point, maybe. Right now, though, it's looking like 10s to 100s of changes will be required to get all the examples to build w/o warning-errors. Some changes will be in the libraries, not just the sample code, too, so it's going to be somewhat intrusive.

I need to find a way to get the Travis tests to dump raw output from arduino-builder to get a good list, because individually building the dozens of examples is painful and slow...

Use platform.local.txt to add -Werror to GCC for the build of all code. Any warnings on a submitted patch will cause an error. Several examples and libraries had warnings/errors (missing returns on functions). Clean those up with this commit as well.

earlephilhower · 2018-02-16T22:48:45Z

This is now completed and there are no errors or warnings reported when run with -Wall -Werror as part of the build process. Ready for review.

Most changes were trivial. A couple library functions had missing returns in non-void functions that were either converted to void fcns()s or updated to actually report success/failure.

The only one that's a little squirrely is in DNS.cpp where they were accessing a byte array as several uint16_t*s. Because the offsets in the byte array were all even it worked, but it does produce a warning in GCC about type-punning. These were replaced by a fully legal (even on odd-offset) memcpy of the 2-bytes to a local uint16_t and using that staging value instead.

devyte · 2018-02-17T04:08:58Z

libraries/TFT_Touch_Shield_V2/TFTv2.cpp

-    XR = constrain(XR, MIN_X,MAX_X);
-    YU = constrain(YU, MIN_Y,MAX_Y);
-    YD = constrain(YD, MIN_Y,MAX_Y);
+    XL = (XL > MAX_X) ? MAX_X : XL;


how about using std::min here?

By the way, why is constraint on MIN_Y being dropped?

GCC is pedantic here. MIN_X/Y =0, and constrain is a macro that compares a to b So you end up with "(uint16_t)XR < 0" as one of the conditions it checks, and that's by definition always false. I will put std::min in on next push.

devyte · 2018-02-17T04:13:25Z

libraries/SD/src/utility/SdFile.cpp

@@ -905,7 +906,7 @@ uint8_t SdFile::rmRfStar(void) {
      if (!f.remove()) return false;
    }
    // position to next entry if required
-    if (curPosition_ != (32*(index + 1))) {
+    if (curPosition_ != (uint32_t)(32*(index + 1))) {


index is uint16_t, and the value is being expanded.
I think this is ok, after reading up on usual arithmetic conversions, but I'm not positive. Please double check that the cast is ok on the final arithmetic result, and that it shouldn't be done directly on index before the arithmetic operations.

Didn't mean to expand it, thought curPosition_ was 32-bits. Will correct to uint16_t.

Misread your comment, will move the cast inside. No 16 bits.

devyte · 2018-02-17T04:18:52Z

tests/common.sh

@@ -40,7 +40,7 @@ function build_sketches()
    local build_arg=$3
    local build_dir=build.tmp
    mkdir -p $build_dir
-    local build_cmd="python tools/build.py -b generic -v -k -p $PWD/$build_dir $build_arg "
+    local build_cmd="python tools/build.py -b generic -v -w all -k -p $PWD/$build_dir $build_arg "


If all warnings are now eliminated, isn't it ok to build with -werror? I'd like to enforce no warnings, sooner rather than later.

It's already done below where platform.local.txt is modified.

Also, that parameter is not passed to GCC but to build.yp,which passes it to arduino-builder, which then uses it to lookup into a GCC setting array. Can't send in -werror (it's included now in platform.local.txt that's generated before running the builder.

devyte · 2018-02-17T04:27:14Z

I'm approving despite my comments, which are minor. My one pet peeve is that I'd like to enforce no warning builds asap (i.e.: -werror), so as to not accumulate again in the future.
I think this usually falls under code quality metrics, so I suggest merging asap.

igrr

LGTM, just one question in TFT library.

igrr · 2018-02-17T04:54:23Z

libraries/ESP8266LLMNR/ESP8266LLMNR.cpp

@@ -242,8 +245,8 @@ void LLMNRResponder::_process_packet() {

    // Header
    uint8_t header[] = {
-        id >> 8, id & 0xff, // ID
-        FLAGS_QR >> 8, 0, // FLAGS
+        (uint8_t)(id >> 8), (uint8_t)(id & 0xff), // ID


since there's a cast to uint8_t now, masking the low byte with & 0xff is not needed.

igrr · 2018-02-17T04:57:58Z

libraries/TFT_Touch_Shield_V2/TFTv2.cpp

-    XR = constrain(XR, MIN_X,MAX_X);
-    YU = constrain(YU, MIN_Y,MAX_Y);
-    YD = constrain(YD, MIN_Y,MAX_Y);
+    XL = (XL > MAX_X) ? MAX_X : XL;


By the way, why is constraint on MIN_Y being dropped?

igrr · 2018-02-17T04:59:31Z

tests/common.sh

@@ -40,7 +40,7 @@ function build_sketches()
    local build_arg=$3
    local build_dir=build.tmp
    mkdir -p $build_dir
-    local build_cmd="python tools/build.py -b generic -v -k -p $PWD/$build_dir $build_arg "
+    local build_cmd="python tools/build.py -b generic -v -w all -k -p $PWD/$build_dir $build_arg "


It's already done below where platform.local.txt is modified.

igrr · 2018-02-17T05:00:20Z

tests/common.sh

@@ -97,6 +103,9 @@ function install_ide()
    mkdir esp8266com
    cd esp8266com
    ln -s $core_path esp8266
+    # Set custom warnings for all builds (i.e. could add -Wextra at some point)


The last part of the comment seems to be outdated as -Werror is already present.

Actually, "-Werror" is included, but the comment says "-Wextra" :) So this is really OK.

I'll stop reviewing PRs until I wake up. Sorry.

earlephilhower force-pushed the werror branch 2 times, most recently from 57ef039 to a558412 Compare February 15, 2018 05:13

earlephilhower force-pushed the werror branch from a558412 to dcf5340 Compare February 15, 2018 05:16

earlephilhower requested a review from igrr February 15, 2018 05:17

earlephilhower force-pushed the werror branch from fe004b3 to 82f719a Compare February 15, 2018 15:26

earlephilhower force-pushed the werror branch 4 times, most recently from 9a316ac to 985ea3a Compare February 16, 2018 21:12

earlephilhower force-pushed the werror branch from 985ea3a to 50b282a Compare February 16, 2018 21:56

earlephilhower requested review from d-a-v and devyte February 16, 2018 22:49

Merge branch 'master' into werror

1990b4d

devyte approved these changes Feb 17, 2018

View reviewed changes

igrr approved these changes Feb 17, 2018

View reviewed changes

Fix minor style and casting issues

4dcf3ae

earlephilhower merged commit f9ac524 into esp8266:master Feb 18, 2018

earlephilhower deleted the werror branch March 9, 2018 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -Werror to acceptance builds for C and CPP #4369

Add -Werror to acceptance builds for C and CPP #4369

earlephilhower commented Feb 15, 2018 •

edited

Loading

earlephilhower commented Feb 15, 2018

earlephilhower commented Feb 15, 2018

d-a-v commented Feb 16, 2018

earlephilhower commented Feb 16, 2018

earlephilhower commented Feb 16, 2018

devyte Feb 17, 2018

igrr Feb 17, 2018

earlephilhower Feb 17, 2018

devyte Feb 17, 2018

earlephilhower Feb 17, 2018

earlephilhower Feb 17, 2018

devyte Feb 17, 2018

igrr Feb 17, 2018

earlephilhower Feb 17, 2018

devyte commented Feb 17, 2018

igrr left a comment

igrr Feb 17, 2018

igrr Feb 17, 2018

igrr Feb 17, 2018

igrr Feb 17, 2018

earlephilhower Feb 17, 2018

igrr Feb 17, 2018

Add -Werror to acceptance builds for C and CPP #4369

Add -Werror to acceptance builds for C and CPP #4369

Conversation

earlephilhower commented Feb 15, 2018 • edited Loading

earlephilhower commented Feb 15, 2018

earlephilhower commented Feb 15, 2018

d-a-v commented Feb 16, 2018

earlephilhower commented Feb 16, 2018

earlephilhower commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte commented Feb 17, 2018

igrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earlephilhower commented Feb 15, 2018 •

edited

Loading