make: fix sign-compare errors by smlng · Pull Request #7994 · RIOT-OS/RIOT

smlng · 2017-11-10T09:01:32Z

PR is required to achieve #7919, and contains several fixes for sign-compare errors

cladmi

Changes look good, murdock still builds correctly, ACK.

smlng · 2017-11-10T10:46:56Z

more to come, here too

miri64 · 2017-11-11T22:38:41Z

Is this still WIP?

Dismissed because of changes after the ACK

smlng · 2017-11-11T22:41:42Z

Is this still WIP?

I think I just fixed the last thing missing to get #7919 ready to merge, so this and #7993 and #7995 are not WIP anymore and all 4 should be merged swiftly before anything/one breaks "-Wall -Wextra" again.

miri64

This PR changes stuff in GNRC and thus can't be merged before #7925 is merged :-/

miri64 · 2017-11-12T17:41:41Z

Test post incoming. Sorry this is the best PR I could think of :-/.

miri64

I'm sorry, but this PR would cause a merge conflict with #7925 if merged, so
we can't merge it right now due to the GNRC merge embargo. If you are able,
please help reviewing all PRs related to #7925 or test it so we can merge this
PR ASAP and lift the embargo.

This is an automated message.

miri64 · 2017-11-12T18:11:09Z

\o/

miri64 · 2017-11-12T18:26:49Z

The thingy is now live so this comment might show up once again in this PR, but after that it shouldn't anymore ;-).

miri64 · 2017-11-27T12:52:06Z

sys/net/gnrc/network_layer/ndp/internal/gnrc_ndp_internal.c


    if (try_long && ((res = gnrc_netapi_get(iface, NETOPT_ADDRESS_LONG, 0,
-                                            l2src, l2src_maxlen)) > max_short_len)) {
+                                            l2src, l2src_maxlen)) > (int)max_short_len)) {


I think without this change this wouldn't collide with #7925. This file is removed in that PR, so if you revert this change it could get in even with the GNRC embargo.

miri64 · 2017-11-27T12:52:28Z

sys/shell/commands/sc_netif.c

                int hl;
                gnrc_ipv6_netif_t *entry;
-                if (((hl = atoi(argv[3])) < 0) || (hl > UINT8_MAX)) {
+                if (((hl = atoi(argv[3])) < 0) || (hl > (int)UINT8_MAX)) {


Same goes for these lines.

smlng · 2017-11-27T13:19:09Z

@miri64, I would still wait until #7925 is merged and rebase afterwards - because removing the changes here makes this PR inconsistent when used on its own with current master.

miri64 · 2017-11-27T14:42:20Z

@miri64, I would still wait until #7925 is merged and rebase afterwards - because removing the changes here makes this PR inconsistent when used on its own with current master.

Alright. Just wanted to help to get this PR faster along (but my hopes for #7925 getting merged today are still high ;-))

cladmi · 2017-11-27T14:43:49Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

-    if (try_long && ((res = gnrc_netapi_get(iface, NETOPT_ADDRESS_LONG, 0,
-                                            l2src, l2src_maxlen)) > max_short_len)) {
+    /* get source address length */
+    int res = gnrc_netapi_get(iface, NETOPT_SRC_LEN, 0,


@smlng seems good to me

@miri64 I think the change has now the same behavior as before but it would be perfect if you could double check this. (the same change is also in gnrc_ndp2.c).

Why is the variable declaration moved down? IMHO (and I stress that this is not a convention of the project but my personal) it is way more readable and comprehensible if it is at the top of the block, like it was before.

None of this changes except the cast in l270 seem to be about sign-compare errors, but maybe I'm mistaken. I'm not a friend of "massive" code restructuring in separate PRs and would prefer to put everything that isn't about the sign-compare error into a separate PR.

(btw: this is also a change that was simplified in #7925, since gnrc_netif(2) allows me to access the L2-address directly).

regarding code restructuring: I agree (my bad, was carried away by the idea of optimising just a little 😉 ), such should be done in separate PRs if anyway.

Lets see what's left of it after merging #7925 and rebasing this one.

miri64 · 2017-11-27T14:55:24Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

-    if (try_long && ((res = gnrc_netapi_get(iface, NETOPT_ADDRESS_LONG, 0,
-                                            l2src, l2src_maxlen)) > max_short_len)) {
+    /* get source address length */
+    int res = gnrc_netapi_get(iface, NETOPT_SRC_LEN, 0,


Why is the variable declaration moved down? IMHO (and I stress that this is not a convention of the project but my personal) it is way more readable and comprehensible if it is at the top of the block, like it was before.

None of this changes except the cast in l270 seem to be about sign-compare errors, but maybe I'm mistaken. I'm not a friend of "massive" code restructuring in separate PRs and would prefer to put everything that isn't about the sign-compare error into a separate PR.

miri64 · 2017-11-27T14:56:31Z

sys/net/gnrc/netif2/gnrc_netif2.c

                res = 0;
-                for (unsigned i = 0;
-                     (res < opt->data_len) && (i < GNRC_NETIF2_IPV6_ADDRS_NUMOF);
+                for (unsigned i = 0; (res < (int)opt->data_len) &&


Here and below: Maybe have the end-condution completely on its own line.

miri64 · 2017-11-27T20:25:11Z

Please rebase

miri64 · 2017-11-27T22:55:51Z

(GNRC embargo is lifted)

smlng · 2017-11-28T10:19:56Z

rebased, may I squash?

Fixed

cladmi · 2017-11-28T10:53:22Z

Please squash

cpu, nrf5x_common: fix sign-compare in periph/flashpage drivers, periph_common: fix sign-compare in flashpage cpu, sam0_common: fix sign-compare error in periph/gpio cpu, cc2538: fix sign-compare in periph/timer cpu, sam3: fix sign-compare in periph/gpio cpu, stm32_common: fix sign-compare in periph/pwm cpu, stm32_common: fix sign-compare in periph/timer cpu, stm32_common: fix sign-compare in periph/flashpage cpu, nrf5x_common: fix sign-compare in radio/nrfmin cpu, samd21: fix sign-compare in periph/pwm cpu, ezr32wg: fix sign-compare in periph/gpio cpu, ezr32wg: fix sign-compare in periph/timer drivers, ethos: fix sign-compare sys, net: fix sign-compare cpu, atmega_common: fix sign-compare error cpu, msp430fxyz: fix sign-compare in periph/gpio boards, msb-430-common: fix sign-compare in board_init driver, cc2420: fix sign-compared sys/net: fix sign-compare in gnrc_tftp driver, pcd8544: fix sign-compare driver, pn532: fix sign-compare driver, sdcard_spi: fix sign-compare tests: fix sign_compare sys/net, lwmac: fix sign_compare pkg, lwip: fix sign-compare boards, waspmote: make CORECLOCK unsigned long to fix sign_compare error tests, sock_ip: fix sign compare tests, msg_avail: fix sign compare tests, sock_udp: fix sign compare boards: fix sign-compare for calliope and microbit matrix

smlng · 2017-11-28T11:09:49Z

rebased and squashed

miri64

ACK

miri64 · 2017-11-28T11:11:51Z

cpu/cc2538/include/cc2538_sys_ctrl.h

-#define sys_clock_freq() ((SYS_CTRL->cc2538_sys_ctrl_clk_ctrl.CLOCK_CTRLbits.OSC ? \
-                           RCOSC16M_FREQ : XOSC32M_FREQ) >> \
-                                SYS_CTRL->cc2538_sys_ctrl_clk_ctrl.CLOCK_CTRLbits.SYS_DIV )
+#define sys_clock_freq() ((uint32_t)\


Weird point to break here IMHO, but okay.

miri64 · 2017-11-28T11:29:07Z

The complaint by Travis is valid, but unrelated (and due to version differences in cppcheck). Let's merge and fix that issue in a separate PR.

miri64 · 2017-11-28T11:35:11Z

See #8164 for the Travis issue fix.

cladmi · 2017-11-28T12:44:13Z

Thanks, we are moving toward -Wall -Wextra -Werror by default :)

smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Nov 10, 2017

smlng mentioned this pull request Nov 10, 2017

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

Merged

smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Nov 10, 2017

cladmi previously approved these changes Nov 10, 2017

View reviewed changes

smlng added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 10, 2017

smlng force-pushed the make/fix/sign_compare branch from a1e205d to a9e59fa Compare November 10, 2017 11:27

smlng changed the title ~~make: fix sign-compare errors~~ [WIP] make: fix sign-compare errors Nov 10, 2017

smlng force-pushed the make/fix/sign_compare branch 5 times, most recently from 82c5df7 to e443610 Compare November 11, 2017 20:53

smlng changed the title ~~[WIP] make: fix sign-compare errors~~ make: fix sign-compare errors Nov 11, 2017

smlng removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 11, 2017

miri64 requested changes Nov 12, 2017

View reviewed changes

miri64 assigned miri64 and cladmi and unassigned miri64 Nov 12, 2017

miri64 requested changes Nov 12, 2017

View reviewed changes

kaspar030 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 13, 2017

miri64 reviewed Nov 27, 2017

View reviewed changes

cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 27, 2017

cladmi reviewed Nov 27, 2017

View reviewed changes

miri64 requested changes Nov 27, 2017

View reviewed changes

smlng force-pushed the make/fix/sign_compare branch from 2b58d37 to 97be415 Compare November 28, 2017 09:39

smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 28, 2017

smlng added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Nov 28, 2017

cladmi previously approved these changes Nov 28, 2017

View reviewed changes

smlng force-pushed the make/fix/sign_compare branch from 3e0fcd0 to e381317 Compare November 28, 2017 11:09

miri64 approved these changes Nov 28, 2017

View reviewed changes

miri64 merged commit bf53c88 into RIOT-OS:master Nov 28, 2017

miri64 mentioned this pull request Nov 28, 2017

pn532: remove redundant #ifdefs #8164

Closed

smlng deleted the make/fix/sign_compare branch November 28, 2017 13:48

kYc0o mentioned this pull request Nov 28, 2017

drivers/pcd8544/pcd8544.c: fix var lengths #7859

Closed

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

Conversation

smlng commented Nov 10, 2017

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 10, 2017

Uh oh!

miri64 commented Nov 11, 2017

Uh oh!

smlng commented Nov 11, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 12, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 12, 2017

Uh oh!

miri64 commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 27, 2017

Uh oh!

miri64 commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 27, 2017

Uh oh!

miri64 commented Nov 27, 2017

Uh oh!

smlng commented Nov 28, 2017

Uh oh!

cladmi commented Nov 28, 2017

Uh oh!

smlng commented Nov 28, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 28, 2017

Uh oh!

miri64 commented Nov 28, 2017

Uh oh!

cladmi commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants