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.
Index: test/Analysis/unreachable-code-path.c
===================================================================
--- test/Analysis/unreachable-code-path.c (revision 151663)
+++ test/Analysis/unreachable-code-path.c (working copy)
@@ -122,3 +122,20 @@
goto d;
f: ;
}
+
+// test11: we can actually end up in the default case, even if it is not
+// obvious: there might be something wrong with the given argument.
+enum foobar { FOO, BAR };
+extern void error();
+void test11(enum foobar fb) {
+ switch (fb) {
+ case FOO:
+ break;
+ case BAR:
+ break;
+ default:
+ error(); // no-warning
+ return;
+ error(); // expected-warning {{never executed}}
+ }
+}
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp (revision 151663)
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp (working copy)
@@ -116,6 +116,14 @@
if (CB->size() > 0 && isInvalidPath(CB, *PM))
continue;
+ // It is good practice to always have a "default" label in a "switch", even
+ // if we should never get there. It can be used to detect errors, for
+ // instance. Unreachable code directly under a "default" label is therefore
+ // likely to be a false positive.
+ if (const Stmt *label = CB->getLabel())
+ if (label->getStmtClass() == Stmt::DefaultStmtClass)
+ continue;
+
// Special case for __builtin_unreachable.
// FIXME: This should be extended to include other unreachable markers,
// such as llvm_unreachable.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
