left shifting a negative number is undefined

[dtkerns]

In lib/signal.c: getskewedframe() there are many instanstances of the construct:

if (v == -1 << X)

where X is some constant number of bits to shift. Unfortunately, left shifting a negative number is undefined.

[bemoody]

Unfortunately, those are just the most obvious examples. There are many other places in the library and applications where shift operators are applied to signed values.

These should be fixed, no doubt about it. But it's the more subtle cases - the ones that don't provoke immediate errors/warnings - that need careful attention.

[dtkerns]

You're suggesting I go understand the code and then fix it ;) ... I was hoping someone that already understood it could/would do that! But you your point is well taken.

[dtkerns]

trying to understand what you're checking for... if I assumed for a moment an arbitrarily large (bit length) int, then if (v == -1 << 15) would be if (v == ...11111000000000000000) .. so what causes v to take that value and satisfy the if? and what is the VFILL macro doing? Seems the shifted one's need to be left truncated to the sizeof(v)

The main issue I have is that, because left shifting a negative number is undefined, each compiler writer is free to implement it as they see fit thus your code potentially behaves differently depending which compiler gets used.

[bemoody]

The value of (-1 << 15) is undefined, but the type of the expression is clearly 'int'. In practice, this expression will evaluate to "-1 times two to the 15th power", or -32768, which is within the range of 'int' on every modern computer. So the intention here is to check whether v equals -32768.

(Yes, I'm handwaving; you're exactly right that this behavior is not required by the C standard, but nonetheless it is the way C compilers work on modern twos-complement architectures, unless the compiler is being intentionally pedantic.)

The reason for this particular bit of code in getskewedframe is a little complicated. The intention of the VFILL macro is to allow certain applications to automatically replace each invalid sample with the previous valid sample. (This feature actually appears to have been broken for some time.)

But in any case, it's safe to assume that anywhere you see "X << N" you should read it as "X times two to the Nth power", and anywhere you see "X >> N" you should read "X divided by two to the Nth power, rounded down."

The "rounded down" aspect, in particular, makes it annoying to try to rewrite these expressions in standard C: "x << 8" means the same as "x * 256" (and modern compilers will generate exactly the same machine code for each), but "x >> 8" does not mean the same as "x / 256".

Look at functions like r212() and w212() for examples of what I mean - it shouldn't be too hard to make these functions standards-conformant, but it's not trivial either.