Appease -Wsign-compare in radix.c

[zec]

Summary

Permit compiling with -W, which comes handy when compiling radix.c in (userland) applications with stricter build policies.

[donner]

May you please provide a full context diff?
I can't see, why the compiler issued a warning and how the fix will break other things (i.e. logic errors)

[zec]

Expand the diff to -U30 per request from donner@.

[zec]

In D29661#665092, @donner wrote:

May you please provide a full context diff?
I can't see, why the compiler issued a warning and how the fix will break other things (i.e. logic errors)

Diff updated, the build breaks at:

../../../net/radix.c:459:13: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

} while (b > (unsigned) x->rn_bit);
         ~ ^ ~~~~~~~~~~~~~~~~~~~~

[donner]

The variable b and the struct member rn_bit are both signed values because they are supposed to hold negative numbers.

sys/net/radix.c

459

In this special case, the negative values of rn_bit are considered as absurd high values, while b is used as a signed value later on.

459

The correct way might be something like

b > x->rn_bit && x->rn_bit >= 0

[zec]

sys/net/radix.c

459

The correct way might be something like

b > x->rn_bit && x->rn_bit >= 0

This is exactly what the original author (@rgrimes? ksklower?) left in a comment a line below. The current form of that condition:

b > (unsigned) x->rn_bit

looks like an optimization which removes one branch. The problem is that the compiler silently converts (signed) b to unsigned, but complaints if -Wsign-compare is turned on. If you're concerned with declaring b as unsigned, we could simply make the current reality more explicit:

(unsigned) b > (unsigned) x->rn_bit

which looks plain ugly to me, but is guaranteed to be a no-op change, and will silence the -Wsign-compare, which is the end goal I want to achieve.

459

In this special case, the negative values of rn_bit are considered as absurd high values, while b is used as a signed value later on.

Without pretending I actually understand everything what the existing code does, I fail to see how b could become negative here, and the fact that it is later used unmodified as an argument to rn_newpair() can't make it negative / overflow either.

[donner]

sys/net/radix.c

459

looks like an optimization which removes one branch.

Yes it is.

The problem is that the compiler silently converts (signed) b to unsigned, but complaints if -Wsign-compare is turned on. If you're concerned with declaring b as unsigned, we could simply make the current reality more explicit:

(unsigned) b > (unsigned) x->rn_bit

which looks plain ugly to me, but is guaranteed to be a no-op change, and will silence the -Wsign-compare, which is the end goal I want to achieve.

This is not logical equivalent to the original statement. It's more

(b > x->rn_bit && x->rn_bit >= 0 && b >= 0) ||
(b < x->rn_bit && x->rn_bit <  0 && b <  0)

[donner]

sys/net/radix.c

459

Without pretending I actually understand everything what the existing code does, I fail to see how b could become negative here, and the fact that it is later used unmodified as an argument to rn_newpair() can't make it negative / overflow either.

You might be right, but I did not have enough knowledge of the code to validate this easily.

[zec]

sys/net/radix.c

459

This is not logical equivalent to the original statement.

The current condition in line 459 is this:

b > (unsigned) x->rn_bit

where, as an alternative to the original diff you objected to, I'm suggesting to make explicit type casting instead of implicit:

(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

[donner]

sys/net/radix.c

459

b > (unsigned) x->rn_bit
(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

b = -2 and x->rn_bit = 1 give false and true, respectively

[zec]

sys/net/radix.c

459

b > (unsigned) x->rn_bit
(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

b = -2 and x->rn_bit = 1 give false and true, respectively

Wrong answer. Pls. compile the snippet below and report back the results.

#include <stdio.h>

int
main(int argc, char **argv)
{

int b, x;

scanf("%d", &b);
scanf("%d", &x);

printf("b: %d x: %d\n", b, x);

if (b > (unsigned) x)
        printf("%d > %d\n", b, x);

if ((unsigned) b > (unsigned) x)
        printf("%d > %d\n", b, x);

}

[donner]

https://blog.regehr.org/archives/268
It's complicated.

#include <stdio.h>

int main(int argc, char **argv)
{
   int b;
   unsigned short x;

   scanf("%d", &b);
   scanf("%hd", &x);

   printf("b: %d x: %d\n", b, x);

   if (b > x)
     printf("%d > %d\n", b, x);
   else
     printf("failed\n");

   if ((unsigned) b > x)
     printf("%d > %d\n", b, x);
   else
     printf("failed\n");

}

So the (unsigned) conversion is also a size conversion (to int). This can become relevant, if somebody try to refine the types of the struct to save memory.
So yes, covert both sides is a solution.