News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

FIxes for some compiler warnings

Started by ceeac, February 09, 2019, 12:57:31 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

This patch fixes warnings reported by -Wignored-qualifiers, -Wunused-variable and -Wint-in-bool-context. The patch also renames the -W switch to -Wextra for clarity.

Ters

The code changes look fine with me (with one exception), although I am a bit puzzled what the deal was with all the casting to constant value types (and one constant pointer). It makes be suspect that they were to silence some other warnings.

The one exception is the change in simvehicle.cc. Either no casting is necessary at all, since get_top() returns an uint8 already, or there really is some weird reason why casting to volatile is necessary. As with all the casting to const, I am not sure why that would be.

I thought the change in simpeople.cc had been done already, or was it only proposed and denied?

Dwachs

thanks, incorporated in r8682.

There was a discussion about converting ``if (dx*dy)'' here: https://forum.simutrans.com/index.php?topic=17221.0
It seems that this file was forgotten.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

#3
QuoteThe one exception is the change in simvehicle.cc. Either no casting is necessary at all, since get_top() returns an uint8 already, or there really is some weird reason why casting to volatile is necessary. As with all the casting to const, I am not sure why that would be.
As far as I am aware volatile should not even be used in Simutrans... I cannot think of a possible use case for it. It cannot be used for thread synchronization and its usual use case involves interrupts or peripheral communication.

Ters

Quote from: DrSuperGood on February 10, 2019, 11:46:52 AM
As far as I am aware volatile should not even be used in Simutrans... I cannot think of a possible use case for it. It cannot be used for thread synchronization and its usual use case involves nterrupts or peripheral communication.
And that is all just for variables. Or possibly rather lvalues. This, I think, was an rvalue. As far as I can google, primitive rvalues can't even be volatile (or const), which I assume is the reason for the warnings motivating this patch.

The change that introduced the cast-to-volatile back in 2012 also moved the call to get_top() from once before the loop to once every iteration. The commit message did in no way explain why. Maybe prissi (who at least was the committer if I remember correctly from yesterday's code archaeology) thought the compiler might have optimized the code to only calling get_top() once without the volatile in there. Or maybe some compiler actually did, or even still do, behave that way. Although I could not see why calling get_top() every iteration would be important. The function did not appear to modify the world state.

prissi

if(dx*dy) was chosen over if(  dx &&  dy  ) on performance reasons. The latter must check if dx and then jump, and only check dy of dx!=0. The other is a multiplication done in a single clock cycle nowadays. (However, with modern computers, this may be irrelevant.)

(volatile int)gr-get_top() was copied from another routine, I think, which needed it, otherwise MSVC (7.x I think) took get_top() out of the loop, but objects could be deleted during the loop, i.e. get_top could change during the loop. Not an issue for this loop though

Ters

Quote from: prissi on February 12, 2019, 02:37:40 PM
if(dx*dy) was chosen over if(  dx &&  dy  ) on performance reasons. The latter must check if dx and then jump, and only check dy of dx!=0. The other is a multiplication done in a single clock cycle nowadays. (However, with modern computers, this may be irrelevant.)

Interesting. GCC apparently considers dx * dy equal enough to dx && dy to emit a warning telling us to use the latter, but it does not generate the same code for it. (Micosoft C++ compiler 19.00 generates the same code as GCC, but issues no warning.) From what I can tell, the former has potential overflow problems, which might be the reason it is discouraged, despite being shorter and possibly faster.

Quote from: prissi on February 12, 2019, 02:37:40 PM(volatile int)gr-get_top() was copied from another routine, I think, which needed it, otherwise MSVC (7.x I think) took get_top() out of the loop, but objects could be deleted during the loop, i.e. get_top could change during the loop. Not an issue for this loop though
That is one ugly compiler bug!

DrSuperGood

Quoteif(dx*dy) was chosen over if(  dx &&  dy  ) on performance reasons. The latter must check if dx and then jump, and only check dy of dx!=0. The other is a multiplication done in a single clock cycle nowadays. (However, with modern computers, this may be irrelevant.)
I do not think there are any noticable performance differences between both implementations. Modern CPUs speculatively execute to reduce pipeline stalls, which is the only advantage to be gained here since logical and has a shorter pipeline depth than integer multiply.

It might be a case to just let the compiler decide and write it in most readable form.
if(dx != 0 && dy != 0)
The comparison to 0 is useful for emphisizing that dx and dy are not bool logical values even if the C++ specification means it is not nescescary.

Ters

Quote from: DrSuperGood on February 13, 2019, 12:22:17 AMIt might be a case to just let the compiler decide and write it in most readable form.
That is generally the best until profiling or testing tells otherwise.