On Mon, Jul 11, 2016 at 12:43 PM, Marek Polacek <pola...@redhat.com> wrote:
> The switch fallthrough has been widely considered a design defect in C, a
> misfeature or, to use Marshall Cline's definition, evil. The overwhelming
> majority of the time you don't want to fall through to the next case, but it
> is
> easy to forget to "break" at the end of the case, making this far too error
> prone. Yet GCC (and probably other compilers, too) doesn't have the ability
> to
> warn in this case. A feature request for such warning was opened back in
> 2002,
> but it's mostly been untouched since. But then the [[fallthrough]] attribute
> was
> approved for C++17 [1], and that's what has got me to do all this.
>
> The following series attempts to implement the fabled -Wswitch-fallthrough
> warning. The warning would be quite easy to implement were it not for control
> statements, nested scopes, gotos, and such. I took great pains to try to
> handle all of that before I succeeded in getting all the pieces in place. So
> GCC ought to do the right thing for e.g.:
>
> switch (n)
> {
> case 1:
> if (n > 20)
> break;
> else if (n > 10)
> {
> foo (9);
> __builtin_fallthrough ();
> }
> else
> foo (8); // warn here
> case 2:;
> }
>
> Since approximately 3% of fall throughs are desirable, I added a new builtin,
> __builtin_fallthrough, that prevents the warning from occurring. It can only
> be used in a switch; the compiler will issue an error otherwise. The new C++
> attribute can then be implemented in terms of this builtin. There was an idea
> floating around about not warning for /* FALLTHRU */ etc. comments but I
> rejected this after I'd spent time thinking about it: the preprocessor can't
> know that we're in a switch statement so we might reject valid programs, the
> "falls through" comments vary wildly, occur even in if-statements, etc. The
> warning doesn't warn when the block is empty, e.g. on case 1: case 2: ... The
> warning does, inevitably, trigger on Duff's devices. Too bad. I guess I'd
> suggest using #pragma GCC warning then. (I think Perl uses the "continue"
> keyword for exactly this purpose -- an interesting idea...)
>
> After I'd completed the warning, I kicked off a bootstrap so as to add various
> gcc_fallthrough calls. There were a good amount of them, as expected. I also
> grepped various FALLTHRU/Falls through/...fall thru.../... comments in config/
> and added gcc_fallthroughs to make it easier for people. Wherever I wasn't
> sure that the gcc_fallthrough was appropriate, I added an 'XXX' comment. This
> must not be relied upon as I don't know most of the compiler code. The same
> goes for relevant libraries where I introduced various gomp_fallthrough
> macros conditional on __GNUC__ (I'd tried to use a configure check but then
> decided I won't put up with all the vagaries of autoconf).
>
> This patch is accompanied by more than 2000 lines of new tests to get the
> warning covered though I'm sure folks will come up with other test cases
> that I hadn't considered (hi Martin S. ;).
>
> This warning is enabled by default for C/C++. I was more inclined to put this
> into -Wall, but our common.opt machinery doesn't seem to allow that (ugh!).
> The clang version I have installed doesn't have this warning so I couldn't
> compare my implementation with others.
I don't like it being default turned on or even part of -Wall. As
evidence all of the patches that comes after this is the main reason
why.
Thanks,
Andrew
>
> I plan to plunge into the C++ [[fallthrough]] thingie after this core part is
> dealt with.
>
> Since the patch is too large, I split it into various parts, the core and then
> various __builtin_fallthrough additions.
>
> This patch has been tested on powerpc64le-unknown-linux-gnu,
> aarch64-linux-gnu,
> x86_64-redhat-linux, and also on powerpc-ibm-aix7.1.0.0, though due to PR71816
> I was unable to bootstrap.
>
> Thanks,
>
> [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0188r0.pdf
>
> Marek
