The International Simutrans Forum

Development => Patches & Projects => Topic started by: An_dz on July 08, 2017, 07:05:49 PM

Title: Fix GCC7 warnings
Post by: An_dz on July 08, 2017, 07:05:49 PM
I already committed r8256 (Macro expansion) and r8262 (int to bool) to fix GCC7 warnings.

There's though another warning that I want to ask here first. The fall through warning.

From GCC 7 Changelog (https://gcc.gnu.org/gcc-7/changes.html#c-family):
QuoteNew command-line options have been added for the C and C++ compilers:
-Wimplicit-fallthrough warns when a switch case falls through. [...] It's possible to suppress the warning by either adding a fallthrough comment [...] This warning is enabled by -Wextra.

From GCC Warning Options (https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html):
Quote-Wimplicit-fallthrough is the same as -Wimplicit-fallthrough=3
[...]
-Wimplicit-fallthrough=3 case sensitively matches one of the following regular expressions:

More info on RedHat blog (https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/)

I added those comments and the warning is useful as I found one instance where a break was really missing.




Here's the reason for r8262:
From GCC 7 Changelog (https://gcc.gnu.org/gcc-7/changes.html#c-family):
QuoteNew command-line options have been added for the C and C++ compilers:
[...]
-Wint-in-bool-context warns about suspicious uses of integer values where boolean values are expected. This warning is enabled by -Wall.

But I ended up finding that I forgot about simpeople.cc, and I noticed that in a second check it does this:
if (dx*dy==0) {
I think that a multiplication is too time consuming and I don't think we can assume every compiler will optimise this to something reasonable, so a better check should be done, like:
if (!dx && !dy) {
if (!(dx && dy)) {
Title: Re: Fix GCC7 warnings
Post by: DrSuperGood on July 08, 2017, 08:28:52 PM
QuoteI think that a multiplication is too time consuming and I don't think we can assume every compiler will optimise this to something reasonable, so a better check should be done, like:
Multiplication is not that time consuming. It is division that is the most time consuming by a huge amount.

Why use a nested if? Since the test is for if either is 0...


if (dx == 0 || dy == 0) {

Title: Re: Fix GCC7 warnings
Post by: isidoro on July 08, 2017, 10:10:54 PM
Quote from: An_dz on July 08, 2017, 07:05:49 PM
But I ended up finding that I forgot about simpeople.cc, and I noticed that in a second check it does this:
if (dx*dy==0) {
I think that a multiplication is too time consuming and I don't think we can assume every compiler will optimise this to something reasonable, so a better check should be done, like:
if (!dx && !dy) {
if (!(dx && dy)) {


Be careful.  If I'm not mistaken, if (dx*dy==0) { is equivalent to if (!dx || !dy) {, and that is also equivalent to if (!(dx && dy)) {.
Title: Re: Fix GCC7 warnings
Post by: An_dz on July 08, 2017, 10:29:11 PM
Ops, true. I knew there was something wrong, but I was looking at the second :D
Title: Re: Fix GCC7 warnings
Post by: Dwachs on July 09, 2017, 12:53:27 PM
Patch looks good. Maybe you coudl split it into two commits: one the actual bug fix (simtool) the other adding the 'FALLTHROUGH' comments?

Also, your int-bool commit also fixed a bug bruecke.cc.
Title: Re: Fix GCC7 warnings
Post by: Ters on July 09, 2017, 01:03:47 PM
I don't like vendor specific magic comments, at least not in a heterogeneous development team. Those not using GCC 7 might break them by not thinking them anything special. Having a macro which expands to this comment, or maybe the more clearly special GCC attribute, will perhaps make it clear that these statements are something special. However, those not using GCC 7 are prone to forgetting to add these.

And why did r8262 change something English to something German?
Title: Re: Fix GCC7 warnings
Post by: An_dz on July 09, 2017, 09:10:41 PM
Quote from: Dwachs on July 09, 2017, 12:53:27 PM
Patch looks good. Maybe you coudl split it into two commits: one the actual bug fix (simtool) the other adding the 'FALLTHROUGH' comments?
Yes, I'll commit the bug fix first.

Quote from: Dwachs on July 09, 2017, 12:53:27 PM
Also, your int-bool commit also fixed a bug bruecke.cc.
I thought it did, but was not sure :D

Quote from: Ters on July 09, 2017, 01:03:47 PM
I don't like vendor specific magic comments, at least not in a heterogeneous development team. Those not using GCC 7 might break them by not thinking them anything special. Having a macro which expands to this comment, or maybe the more clearly special GCC attribute, will perhaps make it clear that these statements are something special. However, those not using GCC 7 are prone to forgetting to add these.
That's a good point, should I use __attribute__ ((fallthrough));?

I think the warning is useful, it helped find a bug and also let others know that fall through was really intended.

Quote from: Ters on July 09, 2017, 01:03:47 PM
And why did r8262 change something English to something German?
Huh? I guess you have translated it yourself because I can't see anything wrong (https://github.com/aburch/simutrans/commit/cb1d42b30217b4495f1cf8830c8c184c68a57ecd).
Title: Re: Fix GCC7 warnings
Post by: Ters on July 10, 2017, 05:37:07 AM
Quote from: An_dz on July 09, 2017, 09:10:41 PM
That's a good point, should I use __attribute__ ((fallthrough));?

It is at least an established way of giving commands to the compiler. The only one for the C++ version the project uses. It is however GCC 7 only. Maybe there is something similar for MS C++ compiler?

Quote from: An_dz on July 09, 2017, 09:10:41 PM
Huh? I guess you have translated it yourself because I can't see anything wrong (https://github.com/aburch/simutrans/commit/cb1d42b30217b4495f1cf8830c8c184c68a57ecd).

is_kartenboden became ist_karten_boden on line 199. The former is more English than the latter. In both cases, a final translation of kartenboden is however missing.
Title: Re: Fix GCC7 warnings
Post by: prissi on July 10, 2017, 06:35:22 AM
Maybe I am too late, but where are the int versus boolean changes as a patch?
Title: Re: Fix GCC7 warnings
Post by: Dwachs on July 10, 2017, 10:12:04 AM
Quote from: prissi on July 10, 2017, 06:35:22 AM
Maybe I am too late, but where are the int versus boolean changes as a patch?
It is r8262, see: https://github.com/aburch/simutrans/commit/cb1d42b30217b4495f1cf8830c8c184c68a57ecd , converting

if (dx*dy)

to

if (dx && dy)

Quote from: Ters on July 10, 2017, 05:37:07 AM
is_kartenboden became ist_karten_boden on line 199. The former is more English than the latter. In both cases, a final translation of kartenboden is however missing.
This is the bug I was talking about earlier: is_kartenboden is an enum const, so if (gr->is_kartenboden) is always true, while ist_karten_boden really asks whether some flag is set. Bad naming, is_kartenboden is not exactly English either.
Title: Re: Fix GCC7 warnings
Post by: An_dz on July 10, 2017, 12:51:23 PM
Quote from: Ters on July 10, 2017, 05:37:07 AM
It is at least an established way of giving commands to the compiler. The only one for the C++ version the project uses. It is however GCC 7 only. Maybe there is something similar for MS C++ compiler?
Visual C++ only accepts C++17 [[fallthrough]]; attribute
Title: Re: Fix GCC7 warnings
Post by: TurfIt on October 03, 2017, 01:13:15 AM
IMHO just using the C++17 [[fallthrough]] attribute should be committed; Simutrans code is a jumble of C/C++ standards anyway.

I tried it with older GCCs (5.3.0 and 6.3.0) and they don't understand it, but simply emit an warning during the compile and carry on.

warning: attributes at the beginning of statement are ignored [-Wattributes]
     [[fallthrough]];
     ^


Cluttering up the warnings for users of old compilers is a fair tradeoff to clean things up for the newer IMHO. And incentive for those to upgrade...
Title: Re: Fix GCC7 warnings
Post by: Ters on October 03, 2017, 05:15:53 AM
GCC 5 is the stable compiler for Gentoo. 4 and 6 appears to be the stable ones for Debian. For Ubuntu, it appears to be GCC 4. That gives some weight towards aiming primarily towards those versions.