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

[Ted Kremenek]

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