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
