tests/driver_at86rf2xx: fix var lengths and unused vars by kYc0o · Pull Request #7860 · RIOT-OS/RIOT

kYc0o · 2017-10-26T00:36:26Z

While compiling tests/driver_at86rf2xx with clang in OS X it discovers several minor issues.

This PR fixes them.

jnohlgard · 2017-10-26T07:23:00Z

tests/driver_at86rf2xx/cmd.c

    int res;
    netdev_ieee802154_t *dev;
-    const size_t count = 2;         /* mhr + payload */
+    size_t count = 2;         /* mhr + payload */


How about static const instead?

Well clang was complaining about the const so that's why I changed, since IMHO there's not necessary. What static would change?

Static const means it won't take up stack space

Well I get the original error if I change it:

/Users/facosta/git/RIOT-OS/RIOT/tests/driver_at86rf2xx/cmd.c:253:25: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] struct iovec vector[count]; ^~~~~ 1 error generated.

Can you make it a macro constant then instead?

#define MAC_VECTOR_SIZE (2)

Doesn't make much sense to use stack space for that constant anyway.

kYc0o · 2017-10-26T12:04:32Z

Well, cppcheck complains about something I was noticed already, but have no clue how it should be solved. Any ideas?

jnohlgard · 2017-10-26T12:55:12Z

tests/driver_at86rf2xx/cmd.c

    uint8_t addr[_MAX_ADDR_LEN];
-    int iface, idx = 2, res;
+    int iface, idx = 2;
+    size_t res;


Why did you need to change this? The parse function returns int

Because of this:

/Users/facosta/git/RIOT-OS/RIOT/tests/driver_at86rf2xx/cmd.c:160:36: error: comparison of integers of different signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare] if ((res <= 0) || (res > sizeof(pan))) { ~~~ ^ ~~~~~~~~~~~ 1 error generated.

It would always make more sense to me to change the comparison than, instead of the type.

miri64 · 2017-11-06T09:51:51Z

tests/driver_at86rf2xx/cmd.c

    int res;
    netdev_ieee802154_t *dev;
-    const size_t count = 2;         /* mhr + payload */
+    size_t count = 2;         /* mhr + payload */


Can you make it a macro constant then instead?

#define MAC_VECTOR_SIZE (2)

Doesn't make much sense to use stack space for that constant anyway.

miri64 · 2017-11-06T09:54:06Z

tests/driver_at86rf2xx/recv.c

    od_hex_dump(buffer + mhr_len, data_len - mhr_len, 0);
    printf("txt: ");
-    for (int i = mhr_len; i < data_len; i++) {
+    for (size_t i = mhr_len; i < data_len; i++) {


Use rather unsigned as a counting variable.

The change here is because the comparison shouldn't be made between two variables of different size. mhr_len and data_len are declared as size_t.

a) where did you get that from? Comparisons between variables of different size shouldn't be a problem at all
b) I think on all our platforms sizeof(size_t) == sizeof(unsigned) is true.

a) where did you get that from? Comparisons between variables of different size shouldn't be a problem at all

I said they shouldn't, because it might lead to errors, if a counter variable is smaller than the variable it's compared with of course it cannot reach the required length. It will only work if the count doesn't exceed it's own type length (which yes, it's mostly what happens).

b) I think on all our platforms sizeof(size_t) == sizeof(unsigned) is true.

I think it's not enough, our code needs to be portable. sizeof() returns size_t, you can find here a small discussion about it.

miri64

Well, cppcheck complains about something I was noticed already, but have no clue how it should be solved. Any ideas?

Since you changed the type of res to size_t (which is an unsigned type) it doesn't make sense to check if it is res is less than 0. That is what cppcheck is complaining about. I guess it wasn't a good idea to change that type after all, so let's go with casting to unsigned for the comparisons clang is complaining about.

kYc0o · 2017-11-07T10:53:03Z

Since you changed the type of res to size_t (which is an unsigned type) it doesn't make sense to check if it is res is less than 0. That is what cppcheck is complaining about. I guess it wasn't a good idea to change that type after all, so let's go with casting to unsigned for the comparisons clang is complaining about.

Then I think we're doing something wrong here, the compiler is stating the obvious, so I'd prefer to make the right comparisons rather than trick the compiler to avoid the warning.

miri64 · 2017-11-07T12:51:29Z

Then I think we're doing something wrong here, the compiler is stating the obvious, so I'd prefer to make the right comparisons rather than trick the compiler to avoid the warning.

if a signed value is on the side that is supposed to be smaller, (e.g. res < size) than it is a right comparison. The range of an unsigned is always larger of its respective signed type.

kYc0o · 2017-11-07T14:26:48Z

if a signed value is on the side that is supposed to be smaller, (e.g. res < size) than it is a right comparison. The range of an unsigned is always larger of its respective signed type.

I agree, but why not just do what's correct? Just compare two variables of the same length and no compiler will complain.

miri64 · 2017-11-07T14:30:29Z

I agree, but why not just do what's correct? Just compare two variables of the same length and no compiler will complain.

Because it wastes memory unnecessarily. Comparing two values of different sizes is still correct if you know what you are doing.

kYc0o · 2017-11-07T14:34:07Z

Because it wastes memory unnecessarily. Comparing two values of different sizes is still correct if you know what you are doing.

This optimisation trick might lead to errors. I prefer to waste 2B for a correct type and length than "expect" we all know what we're doing. I believe compiler warnings are not thrown just for fun...

miri64 · 2017-11-07T16:20:23Z

This optimisation trick might lead to errors. I prefer to waste 2B for a correct type and length than "expect" we all know what we're doing. I believe compiler warnings are not thrown just for fun...

It's not a trick. But you are changing types instead of operation which definitely leads to errors.

miri64 · 2017-11-07T16:20:36Z

(see murdock output)

miri64

ACK when Murdock is happy.

miri64

Re-ACK. Please squash

kYc0o assigned miri64 Oct 26, 2017

jnohlgard reviewed Oct 26, 2017

View reviewed changes

kYc0o mentioned this pull request Oct 26, 2017

"Minor" compiling issues found by clang #7875

Closed

16 tasks

smlng mentioned this pull request Oct 31, 2017

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

Merged

miri64 reviewed Nov 6, 2017

View reviewed changes

miri64 requested changes Nov 6, 2017

View reviewed changes

miri64 approved these changes Nov 7, 2017

View reviewed changes

kYc0o force-pushed the fix_miscelaneous_test_at86rf2xx branch from 30b3188 to d834853 Compare November 7, 2017 17:45

miri64 approved these changes Nov 7, 2017

View reviewed changes

kYc0o force-pushed the fix_miscelaneous_test_at86rf2xx branch from 36c0126 to 3dfd2ac Compare November 7, 2017 18:10

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 7, 2017

kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 7, 2017

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 7, 2017

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 7, 2017

kYc0o force-pushed the fix_miscelaneous_test_at86rf2xx branch from 3dfd2ac to b072c90 Compare November 7, 2017 19:46

tests/driver_at86rf2xx: fix var lengths and unused vars

129c170

kYc0o force-pushed the fix_miscelaneous_test_at86rf2xx branch from b072c90 to 129c170 Compare November 7, 2017 20:09

miri64 merged commit 76f1e9c into RIOT-OS:master Nov 7, 2017

kYc0o deleted the fix_miscelaneous_test_at86rf2xx branch November 7, 2017 20:36

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

Conversation

kYc0o commented Oct 26, 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!

kYc0o commented Oct 26, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

kYc0o commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

kYc0o commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants