Author Topic: Fix GCC7 warnings  (Read 491 times)

0 Members and 1 Guest are viewing this topic.

Offline An_dz

  • Web Admin
  • Administrator
  • *
  • Posts: 2593
  • Total likes: 289
  • Helpful: 89
  • D'oh
    • by An_dz
  • Languages: PT, EN, (it, de)
Fix GCC7 warnings
« 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:
Quote
New 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:
Quote
New 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:
Code: [Select]
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:
Code: [Select]
if (!dx && !dy) {
if (!(dx && dy)) {

Offline DrSuperGood

Re: Fix GCC7 warnings
« Reply #1 on: July 08, 2017, 08:28:52 PM »
Quote
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:
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...

Code: [Select]
if (dx == 0 || dy == 0) {

Offline isidoro

Re: Fix GCC7 warnings
« Reply #2 on: July 08, 2017, 10:10:54 PM »
But I ended up finding that I forgot about simpeople.cc, and I noticed that in a second check it does this:
Code: [Select]
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:
Code: [Select]
if (!dx && !dy) {
if (!(dx && dy)) {

Be careful.  If I'm not mistaken,
Code: [Select]
if (dx*dy==0) { is equivalent to
Code: [Select]
if (!dx || !dy) {, and that is also equivalent to
Code: [Select]
if (!(dx && dy)) {.

Offline An_dz

  • Web Admin
  • Administrator
  • *
  • Posts: 2593
  • Total likes: 289
  • Helpful: 89
  • D'oh
    • by An_dz
  • Languages: PT, EN, (it, de)
Re: Fix GCC7 warnings
« Reply #3 on: July 08, 2017, 10:29:11 PM »
Ops, true. I knew there was something wrong, but I was looking at the second :D

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4222
  • Total likes: 168
  • Helpful: 148
  • Languages: EN, DE, AT
Re: Fix GCC7 warnings
« Reply #4 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.
Parsley, sage, rosemary, and maggikraut.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4674
  • Total likes: 170
  • Helpful: 108
  • Languages: EN, NO
Re: Fix GCC7 warnings
« Reply #5 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?

Offline An_dz

  • Web Admin
  • Administrator
  • *
  • Posts: 2593
  • Total likes: 289
  • Helpful: 89
  • D'oh
    • by An_dz
  • Languages: PT, EN, (it, de)
Re: Fix GCC7 warnings
« Reply #6 on: July 09, 2017, 09:10:41 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.

Also, your int-bool commit also fixed a bug bruecke.cc.
I thought it did, but was not sure :D

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.

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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 4674
  • Total likes: 170
  • Helpful: 108
  • Languages: EN, NO
Re: Fix GCC7 warnings
« Reply #7 on: July 10, 2017, 05:37:07 AM »
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?

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.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 8745
  • Total likes: 305
  • Helpful: 229
  • Languages: De,EN,JP
Re: Fix GCC7 warnings
« Reply #8 on: July 10, 2017, 06:35:22 AM »
Maybe I am too late, but where are the int versus boolean changes as a patch?

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4222
  • Total likes: 168
  • Helpful: 148
  • Languages: EN, DE, AT
Re: Fix GCC7 warnings
« Reply #9 on: July 10, 2017, 10:12:04 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
Code: [Select]
if (dx*dy)
to
Code: [Select]
if (dx && dy)
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.

Offline An_dz

  • Web Admin
  • Administrator
  • *
  • Posts: 2593
  • Total likes: 289
  • Helpful: 89
  • D'oh
    • by An_dz
  • Languages: PT, EN, (it, de)
Re: Fix GCC7 warnings
« Reply #10 on: July 10, 2017, 12:51:23 PM »
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