Add -Wimplicit-fallthrough #69
Conversation
Quuxplusone
force-pushed
the
fallthrough
branch
4 times, most recently
from
e738821
to
63cb73e
Compare
| @@ -486,6 +486,8 @@ EX namespace bt { | |||
| add(-2 * t0 + shift1); | |||
| } | |||
| } | |||
| // WARNING! UNINTENTIONAL FALLTHROUGH?? | |||
| goto fallthru; fallthru: | |||
There was a problem hiding this comment.
Leaving a review comment to call this out even more explicitly. I have no idea what this code is doing, but it kind of looks like it could be a bug.
expansion.cpp
Outdated
| @@ -513,6 +513,7 @@ void celldrawer::do_viewdist() { | |||
| celldistance(c, distance_from == dfPlayer ? cwt.at : currentmap->gamestart()); | |||
| dc = (d != cd) ? 0xFF0000 : 0x00FF00; | |||
| label = its(d); | |||
| break; | |||
| } | |||
| case ncNone: ; | |||
There was a problem hiding this comment.
Consider adding a break; here as well. If another case were added after this one, and you forgot to add the break; in this one, then you'd have a bug. Fortunately, once we enable -Wimplicit-fallthrough, the compiler will catch that particular bug and so it doesn't really matter whether you add the break; here now or wait until the compiler tells you it's needed.
| ld a = -models::model_orientation * degree; | ||
| queuestr(xspinpush0(a, +vid.twopoint_param), vid.xres / 100, "X", ringcolor >> 8); | ||
| queuestr(xspinpush0(a, -vid.twopoint_param), vid.xres / 100, "X", ringcolor >> 8); | ||
| return; |
There was a problem hiding this comment.
Here I just duplicated the code from the following case. Other options would be:
-
goto fallthru; fallthru:as I used above (but I don't recommend this because it connotes ugly hackery) -
factor the shared code out into a function and call that function from both places
Quuxplusone
force-pushed
the
fallthrough
branch
from
63cb73e
to
7ba2ce8
Compare
Quuxplusone
force-pushed
the
fallthrough
branch
3 times, most recently
from
8e2006a
to
0b72d95
Compare
Clang doesn't enable this warning by default, so we pass it explicitly. GCC pre-GCC 7 doesn't recognize the flag, so we can't pass it for GCC. GCC 7 enables it by default as part of -Wextra.
Quuxplusone
force-pushed
the
fallthrough
branch
from
0b72d95
to
3927a51
Compare
|
Successfully rebased on master. The remaining Github CI test failures are due to So @zenorogue ping! |
Not much, I'm afraid. Looks like an intermittent issue to me: there are successful MacOS builds along with a couple failed ones in the same run, and I don't recall anything too different in how Some superficial searching yielded "solutions" mentioning disabling proxy in UPDATE: actually, I'm not quite sure that |
Clang doesn't enable this warning by default, so we pass it explicitly.
GCC pre-GCC 7 doesn't recognize the flag, so we can't pass it for GCC.
GCC 7 enables it by default as part of -Wextra.
Fixes #68.