Fixes for more compiler warnings

[ceeac]

This patch fixes some more compiler warnings, mostly related to unused variables and functions, but also warnings reported by Wclass-memaccess and Wtautological-constant-out-of-range-compare.
The full commit log is available here: https://github.com/ceeac/simutrans/compare/master...ceeac:less-warnings

[Ters]

Other reviewers might find https://stackoverflow.com/questions/45349079/how-to-use-attribute-fallthrough-correctly-in-gcc interesting.

I'm not sure why a decimal constant was changed into a hexidecimal one for motd loading. Neither makes it clear why the value is this strange value.

I don't think putting the return value of fread into len is correct given the way fread is called. The return value will be either 0 or 1, since it returns the number of objects read, not the number of bytes. Swap the second and third argument, and it will be correct.

Why was a lot of UTF-8 related functions just commented out? If we don't need them, there should be no harm in removing them completely. If it turns out that we need them later after all, they can either be found in the file history, or just reimplemented based on UTF-8 specification. They are not that complicated.

In binary_heap_tpl, would it perhaps be better to initialize the member variable in an initializer list?

[Dwachs]

the change to remove unused work_cost in simhalt.cc looks more like a bug, i.e., this workcost has to be used instead of cost?

[prissi]

I wonder why using decimals instead of hexadecimals gives a warning. That compiler seems to be broken for me ...

The workcost is indeed not booked. Changed in r8718

Also why is everyone so bent on fixing warnings for GCC? There are more compiler out there. MSVC emits more than 100 warnings, although not all useful. A lot about are loosing precisions due to cast; something GCC is awful silent about.

[Ters]

Also why is everyone so bent on fixing warnings for GCC?

The right question might be why MSVC users are not bent on fixing warnings. Maybe the warnings are not as visible. Although my sample size is small, I am more alert to warnings that come when I use command-line tools to compile than when compiling inside an IDE.

[prissi]

Without being too cynical: Many GCC users seems to have rather extreme standpoints (like OpenSource licenses commata) and thus are also very much bent on achieving zero warning level skills. Ignoring that Oracle, Digital Mars, Windriver, Microsoft, Intel may give different warnings, since those are not standartised.

[Ters]

Well, projects were this is noticeable are probably libraries, frameworks or servers where bugs quickly become a serious vulnerability to their users. Without a standardized way of suppressing warnings that the developer has checked out as not a problem, the easiest way to get rid of them is to rewrite the code so that the warnings no longer trigger. If warnings are just allowed to accumulate, people stop paying attention to the new ones, as they are a needle in a stack of needles.

[Dwachs]

@prissi: there are still unused variables which point to missing accounting of making stuff public.

Code Select Expand


simhalt.cc:2362:22: warning: unused variable 'woowner' [-Wunused-variable]
                                        player_t *const woowner = wo->get_owner();
                                                        ^
simhalt.cc:2366:19: warning: unused variable 'workcost' [-Wunused-variable]
                                        sint64 const workcost = -welt->scale_with_month_length(cost * welt->get_settings().cst_make_public_months);
                                                     ^
simhalt.cc:2353:13: warning: unused variable 'wo' [-Wunused-variable]
                wayobj_t *wo = NULL;


[prissi]

Thank you, removed in r8739

[An_dz]

Let me revive this topic because I think there are two points worth adding.

As for why 'fixing' warnings, it's because they point to things that can be or lead to undefined behaviour, it compiles because C/C++ is meant to compile even if it might explode. Warnings point that it could explode. While not a standard most compilers agree on nearly all warnings, if one doesn't have a type it's generally just because it's not implemented yet. I've never heard of any compiler that shows a warning for something 'fixed' from a warning of another compiler, though it might have happened in older times.

[...] although not all useful. A lot about are loosing precisions due to cast; something GCC is awful silent about.

That's because we don't pass the flag to check those warnings (-Wconversion). And yes, they are important because the loss of data can lead to errors in other parts of the code. If something was made uint16 it's probably a mistake that out of nowhere it became a sint8.

[Ters]

I've never heard of any compiler that shows a warning for something 'fixed' from a warning of another compiler, though it might have happened in older times.

I could have sworn that has happened on Simutrans just a few years back. it is however quite possible that there were actually two issues, or a bigger issue than either compiler realized. One compiler detected one part, but not the other. The other compiler would have found the other, had it not been for the first part, which confuses it without being recognized as an issue. Fixing just the warning from the first compiler will then cause the second compiler to start issuing warnings. It may also be that the fix done for the first compiler is implementation dependent, which can even cause the other compiler to actually issue an all out error. If the developer's/developers' understanding of C++ doesn't let him/them see the full picture, it can certainly feel like a war of compiler warnings. An even more frustrating one if no developer has all compilers involved in the "war".

That's because we don't pass the flag to check those warnings (-Wconversion).

I guess it's not part of -Wall because it may not always be possible to tell the compiler that you know something it doesn't that makes it safe.

[An_dz]

It may also be that the fix done for the first compiler is implementation dependent, which can even cause the other compiler to actually issue an all out error.

Yes, and on these cases you're going non-standard so it's fine if another implementation complains.

I guess it's not part of -Wall because it may not always be possible to tell the compiler that you know something it doesn't that makes it safe.

Yeah, that's generally the case on many of the warnings compilers issue, sometimes it's intended.

[prissi]

If you ever compile on MSVC, you will get hundred or more warning of losing precision and using unsafe string functions (however, the Simutrans project file just silences them, simply we use portable function to be well portable). And the case when fixing one warning triggered an error was with templates and a clash between GCC 2.9x and GCC4 and MSVC7