signed vs unsigned

[Dirk Hohndel]

I knew those would come back to bite me...

On Wed, Mar 23, 2016 at 09:53:04AM +0100, Robert Helling wrote:
> Dirk,
> 
> you committed
> 
>     Clean up signedness confusion in planner.c
> 
> in which you declared several ints to be unsigned. This breaks at least the ascents in recreational mode, see #1030.

:facepalm:

> I located the problem to be that we have
> 
> unsigned int deltad
> 
> in line 1097 but we do the comparison
> 
> if (deltad - depth > 0)

And this is done as unsigned math because both are ints and the rank of
unsigned int is higher than the rank of signed int. Stupid C standard
(yeah, I know, stupid programmer who didn't pay attention).

There are a couple of ways to fix this, the easiest is most likely to
manually cast deltad to be signed for this part of the math

	if ((int)deltad - depth > 0)

> in line 1103. Now, depth happens to be signed (and it needs to be since we stop ascending when we step through the surface, i.e. reach negative depth which is of course not possible for unsigned values). This above comparison, however, gets casted to unsigned int and thus clearly makes no longer sense. (You can see the ascent problem goes away when deltad is made signed again.
> 
> I could now simply send a patch that does exactly that. But I wonder, what you intended to do with your above patch. It is not the first time I run into troubles when mixing signed and unsigned integers in expressions and in particular in comparisons where the implicit cast is to unsigned (which for me almost never makes sense) and which does not even cause a compiler warning (I think it should, at least it would save me a lot of debugging time, I see, there is -Wconversion).

The reason I did this patch (and a few others like it) is exactly what you
say here. Mixing signed and unsigned doesn't always work the way the
programmer naively expects. And when turning on the warnings for signed
vs. unsigned comparisons or assignments, I tried to clean up all the cases
that the compiler complained about and then tried to make sure that I had
the right casts in place to make sure that the promotion rules would give
us the expected results.

Clearly I did that wrong here.

> Anyway, my conclusion from all this trouble in the past was to _always_ use the signed version since this prevents the trouble. As far as I can see, the potential cost is that the maximally representable number is only half the size but this should not be a problem for us since we stay safely away from those limits.
> 
> So, could you please explain to me your rationale for adding the unsigned statements in your above patch as I am clearly missing something.

We have certain values that are unsigned because of the C standard.
sizeof() for example. We have other values that are semantically always
non-negative (temperature in K, pressure in a vessel).

The initial reason for doing all this "cleanup" work was to silence the
more than a thousand warnings one gets when building for iOS (where these
warnings are forced on). And in the process I tried to be smart and use
signed vs unsigned in ways that made sense given what the variables
actually were supposed to contain.

So in this specific case, the logic went something like this: the
ascent_velocity is always positive, so let's use an unsigned for it.

But that's of course stupid, given the way the values are then used.

I will revert the offending commit and try to be more careful the next
time I clean this up (I did the same to two other commits where I got
things wrong earlier...)

Thanks for catching this.

/D