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


Coming back to this...

On Tue, Jul 12, 2016 at 03:00:43PM -0600, Martin Sebor wrote:
> > 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.

Sure it is, thanks.  Since I've replaced __builtin_fallthrough ()
with an attribute, most of your observations should be fixed now.
But there was an ICE happening even with __attribute__((fallthrough)),
which I fixed now.  Your testcase is included (in a bit different
form) in the latest patch submission.

	Marek


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