[cfe-commits] [scan-build][PATCH 1/1] Unreachable code checker : false positive in switch statements ?

[David Blaikie]

Should we just change the way the CFG is built to include edges for
default labels on covered switches all the time? That would fix the
same bug (I'm on the fence about whether it is a bug, myself) in
Clang's -Wunreachable-code.

Presumably we're actually doing work/have code designed to check
covered switches & skip the default edge - we could just remove that
code I would imagine.

(one possible problem: that would then mean the default case is
considered reachable - for reachable-based warnings in
AnalysisBasedWarnings. Do these default cases fall into the special
"neither provably reachable nor unreachable" middle ground (like the
unreachable code in template instantiations that isn't unreachable in
the pattern, or code that's unreachable on the basis of sizeof
expressions, macros, etc))

On Tue, Feb 28, 2012 at 10:10 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Cyril,
>
> I agree.  Applied in r151709!
>
> Thanks so much,
> Ted
>
> On Feb 28, 2012, at 5:45 PM, Cyril Roelandt <tipecaml at gmail.com> wrote:
>
>> Hello,
>>
>> While trying out the UnreachableCode checker of scan-build, I noticed that it would report an unreachable statement in the following code :
>>
>> enum foobar { FOO, BAR };
>> extern void error();
>> void test(enum foobar fb) {
>>  switch (fb) {
>>    case FOO:
>>      break;
>>    case BAR:
>>      break;
>>    default:
>>      error();
>>  }
>> }
>>
>>
>> enum.c:10:7: warning: This statement is never executed
>>      error();
>>      ^~~~~
>>
>> Here, scan-build seems to be right. In the test() function, the argument "fb" can only have two different values (FOO and BAR), which means we should never get into the "default" case. Still, many programmers will use this default case, for three reasons :
>>
>> 1) A switch statement without a default case looks weird : it is an uncommon pattern.
>> 2) One could call test() with an argument that is neither FOO, neither BAR, like test(42). Here, the default case allows programmers to easily catch errors : it is more elegant than writing if (fb != FOO && fb != BAR) { ... } at the beginning of the test() function.
>> 3) If a "FOOBAR" field is ever added to the "enum foobar", then all functions using variables of type "enum foobar" (such as test()) will have to be updated. If programmers forget one, the program will end up in the default case and trigger an error. Keeping the default case is an easy way to find such mistakes.
>>
>> For all these reasons, I think that "error();" should not be reported as an unreachable statement, for it is probably a false positive.
>>
>> Obviously, in the following code :
>>
>> default:
>>  foo();
>>  break;
>>  bar();
>>
>> "bar();" should be reported as an unreachable statement.
>>
>>
>> I have updated the tests to take this into account, and written a very simple patch that seems to do the job. I am not sure it is the best way to go, but it should illustrate my point and was not too hard to write for an unexperienced clang hacker like me :) I would like to hear what you think about it. The whole thing comes attached as a patch.
>>
>>
>> Looking forward to hearing from you,
>> Cyril Roelandt.
>> <unreachable_code_default_statement.patch>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits