Topic: Fixes for left shift of negative values

[ceeac]

Continuation of https://forum.simutrans.com/index.php/topic,19115.0.html

These are some fixes for left shifts of negative signed integers in simgraph16.cc. From my tests, the assembly output is the same for gcc/clang/msvc, so program behaviour should not be affected.

[DrSuperGood]

I was not aware such shifts were undefined behaviour lol. Guess I became too complacent with languages like Java where such shifts are well defined.

This might effect debug performance as the multiplication cannot be optimized away. However in release mode any sensible optimizer should optimize them away if the platform offered bitshift instructions allow it.

[Ters]

Is this because C doesn't require negative integers to be represented with two's complement? I don't think it is a big problem that Simutrans won't work on architectures that don't use that. Do some compilers actually give warnings for this? GCC 8.3.0 doesn't even say anything with pedantic warnings turned on. Then again, it does't warn about the right-hand operand possibly being negative either (only when it is absolutely sure it is). That, I think, is plain impossible on x86.

[Dwachs]

The sanitizer-compiled builds warn about this at runtime.

[Ters]

Is there no right-shifting of negative values in Simutrans? I would think those were worse, since the logic for doing that is different even with two's complement. x86 therefore has two different right-shift instructions, but only one for left. Or have right-shifts been changed likewise already?

[DrSuperGood]

Is there no right-shifting of negative values in Simutrans?

The most likely place for such shifts would be with all the emulated fixed point maths for industry and the like. Simutrans really should be using its own fixed point library...

Fortunately most of the maths do not involve negative numbers, which is likely why this is not an issue.

When dealing with the graphics, especially pixel decoding or packing, unsigned shifts should be used as the integer is being treated as a field of bits and not a numeric value.

[Ters]

The shifts in the patch looked very much like fixed point to me. More so with the original code than with the multiplication thrown in. While the two do functionally the same, how it is written can tell something about why it does what it does. It would have been clearer if Simutrans had a dedicated fixed-point type.

[prissi]

While technically true, any machine with char != 8 bits and single complement will fail with Simutrans compilation. So this is rather a non-issue.

Moreover, the Bresenham algorithm is one of the oldest line drawing algorithm and is used in zillions of programs, even on ancient machines. If that could ever fail, then I have never heard of it.