This is the mail archive of the
mailing list for the GCC project.
Re: Implement -Wswitch-fallthrough
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Marek Polacek <polacek at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 12 Jul 2016 12:27:31 +0200
- Subject: Re: Implement -Wswitch-fallthrough
- Authentication-results: sourceware.org; auth=none
- References: <20160711194340.GI13963@redhat.com>
On Mon, Jul 11, 2016 at 9:43 PM, Marek Polacek <firstname.lastname@example.org> 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 , and that's what has got me to do all this.
I don't like this too much given the churn it requires in GCC itself.
was approved for C++17 is there sth similar proposed for C? Like a keyword
> 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)
> else if (n > 10)
> foo (9);
> __builtin_fallthrough ();
> 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 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-aix220.127.116.11, though due to PR71816
> I was unable to bootstrap.
>  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0188r0.pdf