Bump with updated patch (fixes existing test cases that triggered this
warning (well, suppressed the warning in those cases) & added one for
the new warning, added a separate flag for the warning
(-Wswitch-enum-redundant-default (suggestions welcome)) and grouped it
under -Wswitch-enum). I've also used this warning to fix all the cases
of this in LLVM & Clang (changes already checked in - except for some
cases in tblgen-erated code and google test)
- David
On Tue, Jan 10, 2012 at 12:24 AM, David Blaikie <[email protected]> wrote:
> <bump>
>
> On Wed, Dec 14, 2011 at 12:12 PM, David Blaikie <[email protected]> wrote:
>> On Sat, Sep 24, 2011 at 7:21 AM, Ted Kremenek <[email protected]> wrote:
>>> On Sep 23, 2011, at 9:33 PM, David Blaikie wrote:
>>>
>>>> Hi Ted,
>>>>
>>>> I tried -Wunreachable-code earlier today (Chandler had suggested it as a
>>>> way to find/remove the dead code after llvm_unreachables I'd migrated
>>>> yesterday) & it produced some very... interesting output. It did find the
>>>> dead code after llvm_unreachable but it also found some other very strange
>>>> cases. I was wondering what was up with that. Good to know it's WIP - any
>>>> tips on the state of that? anywhere I'd be able to lend a hand?
>>>
>>> Hi David,
>>>
>>> The weirdest results I see from -Wunreachable-code tend to involve template
>>> code. In templates, I've seen cases where a branch condition depends on a
>>> template parameter, and at template instantiation the branch condition may
>>> become a constant. This can cause some code to (correctly) be flagged as
>>> unreachable for that particular instantiation of a template. This of
>>> course is not an invariant for that template for *all* instantiations, so
>>> it's not a real issue.
>>>
>>> The solution I had in mind to fix this problem is to enhance the CFG.
>>> Instead of just pruning CFG edges, for templates we could record when an
>>> edge was pruned AND whether or not it was dependent on a template
>>> parameter. Most analyses would continue to see the CFG as they do now,
>>> but specific analyses (such as -Wunreachable-code) could do something a bit
>>> more clever and not treat such code as unreachable.
>>>
>>>>
>>>> It wouldn't catch all the same cases ("case N: default: stuff" for
>>>> example) but this solution isn't great either, it'll catch a variety of
>>>> arcane cases that won't have trivial fixes.
>>>
>>> Ah, interesting. -Wunreachable-code looks for finding unreachable basic
>>> blocks, not looking at whether or not a label could never be used. Those
>>> are different concepts.
>>>
>>>> Chandler had mentioned the idea of this warning (well, something like it)
>>>> yesterday but after I threw this together we were talking about it more &
>>>> realized it'd be pretty tricky to get right with a nice multiline fixit
>>>> that is very reliable (I get the impression that's what he's really
>>>> interested in - really low (0?) false positive rate & accurate fixits -
>>>> which would be awesome, but require a rather different fix)
>>>
>>> I agree. A warning like this in the compiler needs close to a zero false
>>> positive rate.
>>
>> I'm just coming back to this (checking all my "loose ends" threads, or
>> at least some of them) and I can't quite remember what the problem was
>> here. Is there a case my warning would fire on that shouldn't be
>> fixed? I don't think so. The real issue would be the fixit - removing
>> the default label would always be valid, but that could introduce
>> (probably fairly trivial) unreachable code. Adding a fixit to remove
>> the whole block would be harder. Should we have the warning with no
>> fixit at all?
>>
>> Also, I'm still interested in checking in the mechanical fixes & so
>> far as I know it's the intended/preferred way of writing switches in
>> Clang - it's usually given as code review feedback, but as with
>> everything that isn't verified, things slip through. So if someone
>> wants to confirm/sign off on that I'll check those in & we can nut out
>> the warning separately.
>>
>> - David
diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index 9c03580..ca0ce99 100644
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -178,7 +178,8 @@ def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
def : DiagGroup<"strict-prototypes">;
def StrictSelector : DiagGroup<"strict-selector-match">;
def MethodDuplicate : DiagGroup<"duplicate-method-match">;
-def SwitchEnum : DiagGroup<"switch-enum">;
+def SwitchEnumRedundantDefault : DiagGroup<"switch-enum-redundant-default">;
+def SwitchEnum : DiagGroup<"switch-enum", [SwitchEnumRedundantDefault]>;
def Switch : DiagGroup<"switch", [SwitchEnum]>;
def Trigraphs : DiagGroup<"trigraphs">;
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index f685c9f..3ffd580 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4946,19 +4946,22 @@ def warn_case_empty_range : Warning<"empty case range specified">;
def warn_missing_case_for_condition :
Warning<"no case matching constant switch condition '%0'">;
def warn_missing_case1 : Warning<"enumeration value %0 not handled in switch">,
- InGroup<DiagGroup<"switch-enum"> >;
+ InGroup<SwitchEnum>;
def warn_missing_case2 : Warning<
"enumeration values %0 and %1 not handled in switch">,
- InGroup<DiagGroup<"switch-enum"> >;
+ InGroup<SwitchEnum>;
def warn_missing_case3 : Warning<
"enumeration values %0, %1, and %2 not handled in switch">,
- InGroup<DiagGroup<"switch-enum"> >;
+ InGroup<SwitchEnum>;
def warn_missing_cases : Warning<
"%0 enumeration values not handled in switch: %1, %2, %3...">,
- InGroup<DiagGroup<"switch-enum"> >;
+ InGroup<SwitchEnum>;
+def warn_unreachable_default : Warning<
+ "default is unreachable as all enumeration values are accounted for">,
+ InGroup<SwitchEnumRedundantDefault>;
def warn_not_in_enum : Warning<"case value not in enumerated type %0">,
- InGroup<DiagGroup<"switch-enum"> >;
+ InGroup<SwitchEnum>;
def err_typecheck_statement_requires_scalar : Error<
"statement requires expression of scalar type (%0 invalid)">;
def err_typecheck_statement_requires_integer : Error<
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index a53e397..0be3e43 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -924,11 +924,17 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
if (RI == CaseRanges.end() || EI->first < RI->first) {
hasCasesNotInSwitch = true;
- if (!TheDefaultStmt)
- UnhandledNames.push_back(EI->second->getDeclName());
+ UnhandledNames.push_back(EI->second->getDeclName());
}
}
+ if (TheDefaultStmt) {
+ if (UnhandledNames.size() == 0)
+ Diag(TheDefaultStmt->getDefaultLoc(), diag::warn_unreachable_default);
+ else
+ UnhandledNames.clear();
+ }
+
// Produce a nice diagnostic if multiple values aren't handled.
switch (UnhandledNames.size()) {
case 0: break;
diff --git a/test/Sema/switch.c b/test/Sema/switch.c
index ef2e4c5..63ffed1 100644
--- a/test/Sema/switch.c
+++ b/test/Sema/switch.c
@@ -288,3 +288,12 @@ void test17(int x) {
case 0: return;
}
}
+
+int test18() {
+ enum { A, B } a;
+ switch (a) {
+ case A: return 0;
+ case B: return 1;
+ default: return 2; // expected-warning {{default is unreachable as all enumeration values are accounted for}}
+ }
+}
diff --git a/test/Sema/warn-unreachable.c b/test/Sema/warn-unreachable.c
index 3ad53c7..46a680f 100644
--- a/test/Sema/warn-unreachable.c
+++ b/test/Sema/warn-unreachable.c
@@ -1,4 +1,4 @@
-// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code -Wno-unused-value
+// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code -Wno-unused-value -Wno-switch-enum-redundant-default
int halt() __attribute__((noreturn));
int live();
diff --git a/test/SemaCXX/gnu-case-ranges.cpp b/test/SemaCXX/gnu-case-ranges.cpp
index c1c18a8..94100d2 100644
--- a/test/SemaCXX/gnu-case-ranges.cpp
+++ b/test/SemaCXX/gnu-case-ranges.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -Wno-switch-enum-redundant-default %s
enum E {
one,
diff --git a/test/SemaCXX/return-noreturn.cpp b/test/SemaCXX/return-noreturn.cpp
index e06ba40..4b1f82d 100644
--- a/test/SemaCXX/return-noreturn.cpp
+++ b/test/SemaCXX/return-noreturn.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -Wreturn-type -Wmissing-noreturn -Wno-unreachable-code
-// RUN: %clang_cc1 %s -fsyntax-only -std=c++11 -verify -Wreturn-type -Wmissing-noreturn -Wno-unreachable-code
+// RUN: %clang_cc1 %s -fsyntax-only -verify -Wreturn-type -Wmissing-noreturn -Wno-unreachable-code -Wno-switch-enum-redundant-default
+// RUN: %clang_cc1 %s -fsyntax-only -std=c++11 -verify -Wreturn-type -Wmissing-noreturn -Wno-unreachable-code -Wno-switch-enum-redundant-default
// A destructor may be marked noreturn and should still influence the CFG.
void pr6884_abort() __attribute__((noreturn));
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
