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

[Abramo Bagnara]

Il 28/10/2012 13:27, Matthieu Monrocq ha scritto:
> 
> 
> On Sun, Oct 28, 2012 at 12:08 PM, Abramo Bagnara
> <abramo.bagnara at bugseng.com <mailto: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>
>     <mailto: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.

The fact that the switch is "perfectly" covered means only that every
enum constant listed is handled in case statements. This says nothing
about the fact that the default might be needed for valid values in the
enum range but not present in enum constant list.

Note that the default branch in the example above is perfectly reachable
and is subject to code generation (it cannot be optimized away and it is
not optimized away with clang or gcc or other compilers).

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

I'm inclined to disagree, these two warning show very different infos:

-Wunreachable: this code is never executeed and might be removed from
code generation

-Wcovered-switch-default: the default might be needless if the enum
typed switch guard is expected to not have any value different from enum
constants listed in the enum definition

Of course there are marginal cases where both condition are true (e.g.
an enum with unsigned char underlying type and all enum constants with
value from 0 to 255 are represented in case statements, but this is not
really relevant.

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

This would not help, the default is reachable and subject to code
generation with and without int cast.


-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara at bugseng.com