Skip to content

Conversation

@Quuxplusone
Copy link
Contributor

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.

@Quuxplusone Quuxplusone force-pushed the fallthrough branch 4 times, most recently from e738821 to 63cb73e Compare March 29, 2020 14:16
}
}
// WARNING! UNINTENTIONAL FALLTHROUGH??
goto fallthru; fallthru:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
label = its(d);
break;
}
case ncNone: ;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Quuxplusone force-pushed the fallthrough branch 3 times, most recently from 8e2006a to 0b72d95 Compare September 28, 2020 19:41
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
Copy link
Contributor Author

Successfully rebased on master. The remaining Github CI test failures are due to brew update timing out (what should we do about this recurring problem, @still-flow?) and the MinGW static initialization order fiasco (which I keep meaning to investigate — I mean the whole idea of initializing a global to geometry[ginf] seems wrong, because that value will be out-of-date immediately — but I haven't really tackled it yet).

So @zenorogue ping!

@still-flow
Copy link
Contributor

still-flow commented Sep 29, 2020

brew update timing out (what should we do about this recurring problem, @still-flow?)

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 homebrew is handled from job to job -- after all, each job is supposed to run in a separate VM starting from the same image.

Some superficial searching yielded "solutions" mentioning disabling proxy in git config or overly-keen firewalls/antiviruses, but the responses were inconclusive. But given what I've said above, it's probably worth to report this to Github support.

UPDATE: actually, I'm not quite sure that brew update is all that necessary. Don't know the specifics, but it might be the case that the package database is already present in the VM image, even if slightly outdated. So removing this step might help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't compile on Linux Mint 19.1.

2 participants