This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Implement -Wswitch-fallthrough


On Mon, Jul 11, 2016 at 9:43 PM, Marek Polacek <polacek@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.

I don't like this too much given the churn it requires in GCC itself.
If [[fallthrough]]
was approved for C++17 is there sth similar proposed for C?  Like a keyword
__Fallthrough?

Richard.

> 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 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]