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

[Jordan Rose]

On Oct 30, 2012, at 10:17 , David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Oct 30, 2012 at 9:25 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>> On Oct 30, 2012, at 2:34 , Abramo Bagnara <abramo.bagnara at bugseng.com> wrote:
>> 
>>> Il 28/10/2012 08:12, Abramo Bagnara ha scritto:
>>>> $ 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).
>>> 
>>> I've attached the patch for review.
>>> 
>>> The fixed testcase shows well why to hide warnings about undefined
>>> behaviour in code actually generated is a bad thing.
>> 
>> If we do this, we're going to want this under a CFG option at the very least. The static analyzer should continue assuming that an enum input to a switch is always a valid enum constant, in order to keep our false positive rate down.
> 
> Yeah, I doubt this'll be any better in the compiler proper, really.
> The heuristic exists to, as you rightly point out, reduce false
> positives & that rationale exists in the compiler as well.
> 
> While, yes, it means we lose some true positives as well, that
> probably isn't worth the increase in false positives.

I can see Abramo's point, however, that in a more defensive coding style the current -Wunreachable warning can easily be considered a false positive. We don't optimize out the default case in an enum-covered switch.