[cfe-dev] False positive for -Wunreachable-code

[Matthieu Monrocq]

On Sun, Oct 28, 2012 at 12:08 PM, Abramo Bagnara <abramo.bagnara at bugseng.com
> wrote:

> Il 28/10/2012 11:47, Matthieu Monrocq ha scritto:
> >
> >
> > On Sun, Oct 28, 2012 at 8:12 AM, Abramo Bagnara
> > <abramo.bagnara at bugseng.com <mailto:abramo.bagnara at bugseng.com>> wrote:
> >
> >     $ cat p.c
> >     #include <stdio.h>
> >
> >     enum e { a, b = 4 } x = 3;
> >
> >     void g(int v) {
> >       printf("%d\n", v);
> >     }
> >
> >     int main(int argc, char **argv) {
> >       switch (x) {
> >       case a:
> >         g(0);
> >         break;
> >       case b:
> >         g(1);
> >         break;
> >       default:
> >         g(2);
> >         break;
> >       }
> >     }
> >     $ _clang -Wunreachable-code -Wcovered-switch-default -O2 p.c
> >     p.c:17:3: warning: default label in switch which covers all
> enumeration
> >     values
> >           [-Wcovered-switch-default]
> >       default:
> >       ^
> >     p.c:18:7: warning: will never be executed [-Wunreachable-code]
> >         g(2);
> >           ^
> >     $ ./a.out
> >     2
> >
> >     Of course -Wcovered-switch-default warning is a perfectly true
> positive.
> >
> >     My reading of the standard is that nothing prevent an enum to have a
> >     value different from listed enum constants if this value is
> compatible
> >     with enum range (and code generation seems to agree on that).
> >
> >     Are there any objection to file a bug report?
> >
> >
> > Well, I would object on the basis that for all "regular" uses of the
> > switch/enum combination: ie, the enum values are only the enumerators
> > and the switch does cover all enumerators; then this warning is
> > perfectly valid.
> >
> > Therefore I would rather say that people who use enums for bit-masks and
> > thus have variables of the enum type with values other than the
> > enumerators (which is fine) should either not be using a switch on this
> > enum OR simply disable this warning. Maybe casting `x` to `int` such as
> > `switch(int(x))` would also work.
> >
> > How do you propose to distinguish the two widely different usages of
> enum ?
> >
> > I personally would be rather disappointed to see
> > -Wcovered-switch-default disappear since at both the company I work on
> > and in my pet programs we only ever use enum for its enumerators and
> > thus this warning is very useful.
>
> There is a misunderstanding here:
>
> - I think that -Wcovered-switch-default is very useful and the warning
> show above is a *TRUE* positive, so there is nothing to fix there
>
> - my point is that -Wunreachable-code is showing a false negative for
> the example above and should be fixed
>
>
I am afraid I did not expressed myself clearly either: if we accept that
the switch is perfectly covered (and thus warn that the default is not
needed), then it falls out that the code in the default branch is
unreachable.

I am fine to remove the `-Wunreachable` warning in the case were the
`-Wcovered-switch-default` one has already been emitted, they are redundant.

The real question is: when Clang does not warn about the switch already
being covered, should it warn about the code being unreachable ?

What do you think of a note suggesting a cast to the underlying integral
type of `x` such as `switch(int(x))` added to `-Wunreachable` to show how
to silence it ? (and making it so that it does silence it)

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121028/5b982ab2/attachment.html>