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


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.

I think this is a useful feature though like others I'm not
entirely sure that a built-in is the right interface.  I think
I would find a pragma or an attribute preferable.  At a minimum,
it would obviate some questions raised by my testing (i.e.,
whether the built-in be accepted when an attribute would
otherwise not be syntactically allowed).

I applied the core patch and ran a few experiments with it on
the assumption that __builtin_fallthrough() should behave roughly
correspondingly to [[fallthrough]].  I.e., be rejected where
[[fallthrough]] is rejected (but where attributes are otherwise
valid) and be accepted where the latter is accepted.  This may
not be intended but the text added to the manual is too vague
to tell.  I also compared the results to Clang's [[fallthrough]]
to make sure I wasn't missing something (or too much).

I ran into a few of what seems like subtle bugs or limitations
some of which I'm not sure are going to be solvable in the middle
end (at least not after the control flow graph has been built)
because by the time the middle end sees the code (certainly by
the time it gets to expansion) some of it has been eliminated.
An illustrative example of this class of problems might be this
function:

  void f (void)
  {
    if (0) __builtin_fallthrough ();   // never diagnosed

    int i = 0;
    if (i) __builtin_fallthrough ();   // not diagnosed with -O
  }

With the built-in replaced by [[fallthrough]] Clang diagnoses
both of these.

This may be an okay limitation for __builtin_fallthrough since
it's GCC-specific, but it won't do for the C++ attribute or for
the C attribute if one is added with matching constraints.

The tests I tried are in the attached file.  Most of them are
edge cases but some I think point out more serious problems
(the checker getting confused/disabled by a prior switch
statement).

Hopefully this will be useful.

Martin

Attachment: t.c
Description: Text document


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