News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

Fix GCC7 warnings

Started by An_dz, July 08, 2017, 07:05:49 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

An_dz

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:
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:
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

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:
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)) {

DrSuperGood

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) {


isidoro

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)) {.

An_dz

Ops, true. I knew there was something wrong, but I was looking at the second :D

Dwachs

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.
Parsley, sage, rosemary, and maggikraut.

Ters

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?

An_dz

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.

Ters

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.

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.

prissi

Maybe I am too late, but where are the int versus boolean changes as a patch?

Dwachs

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.
Parsley, sage, rosemary, and maggikraut.

An_dz

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

TurfIt

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...

Ters

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.