From: John McCall Date: Tue, 18 May 2010 03:19:21 +0000 (+0000) Subject: If a switch condition is constant, don't warn about missing enum cases. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0fb97083cc0f8a82e404e22991ae80d2216e71d5;p=clang If a switch condition is constant, don't warn about missing enum cases. If a switch condition is constant, warn if there's no case for it. Constant switch conditions do come up in reasonable template code. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@104010 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 65f72167f1..2298f6a3e9 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -2892,9 +2892,11 @@ def warn_case_value_overflow : Warning< InGroup>; def err_duplicate_case : Error<"duplicate case value '%0'">; 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_cases : Warning<"enumeration value %0 not handled in switch">, InGroup >; -def not_in_enum : Warning<"case value not in enumerated type %0">, +def warn_not_in_enum : Warning<"case value not in enumerated type %0">, InGroup >; def err_typecheck_statement_requires_scalar : Error< "statement requires expression of scalar type (%0 invalid)">; diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 3200288e43..875b160d71 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -567,6 +567,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, } Expr *CondExpr = SS->getCond(); + Expr *CondExprBeforePromotion = CondExpr; QualType CondTypeBeforePromotion = GetTypeBeforeIntegralPromotion(CondExpr); @@ -675,16 +676,38 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, } if (!HasDependentValue) { + // If we don't have a default statement, check whether the + // condition is constant. + llvm::APSInt ConstantCondValue; + bool HasConstantCond = false; + bool ShouldCheckConstantCond = false; + if (!HasDependentValue && !TheDefaultStmt) { + Expr::EvalResult Result; + HasConstantCond = CondExprBeforePromotion->Evaluate(Result, Context); + if (HasConstantCond) { + assert(Result.Val.isInt() && "switch condition evaluated to non-int"); + ConstantCondValue = Result.Val.getInt(); + ShouldCheckConstantCond = true; + + assert(ConstantCondValue.getBitWidth() == CondWidth && + ConstantCondValue.isSigned() == CondIsSigned); + } + } + // Sort all the scalar case values so we can easily detect duplicates. std::stable_sort(CaseVals.begin(), CaseVals.end(), CmpCaseVals); if (!CaseVals.empty()) { - for (unsigned i = 0, e = CaseVals.size()-1; i != e; ++i) { - if (CaseVals[i].first == CaseVals[i+1].first) { + for (unsigned i = 0, e = CaseVals.size(); i != e; ++i) { + if (ShouldCheckConstantCond && + CaseVals[i].first == ConstantCondValue) + ShouldCheckConstantCond = false; + + if (i != 0 && CaseVals[i].first == CaseVals[i-1].first) { // If we have a duplicate, report it. - Diag(CaseVals[i+1].second->getLHS()->getLocStart(), - diag::err_duplicate_case) << CaseVals[i].first.toString(10); Diag(CaseVals[i].second->getLHS()->getLocStart(), + diag::err_duplicate_case) << CaseVals[i].first.toString(10); + Diag(CaseVals[i-1].second->getLHS()->getLocStart(), diag::note_duplicate_case_prev); // FIXME: We really want to remove the bogus case stmt from the // substmt, but we have no way to do this right now. @@ -703,6 +726,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, // Scan the ranges, computing the high values and removing empty ranges. std::vector HiVals; for (unsigned i = 0, e = CaseRanges.size(); i != e; ++i) { + llvm::APSInt &LoVal = CaseRanges[i].first; CaseStmt *CR = CaseRanges[i].second; Expr *Hi = CR->getRHS(); llvm::APSInt HiVal = Hi->EvaluateAsInt(Context); @@ -718,7 +742,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, CR->setRHS(Hi); // If the low value is bigger than the high value, the case is empty. - if (CaseRanges[i].first > HiVal) { + if (LoVal > HiVal) { Diag(CR->getLHS()->getLocStart(), diag::warn_case_empty_range) << SourceRange(CR->getLHS()->getLocStart(), CR->getRHS()->getLocEnd()); @@ -726,6 +750,12 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, --i, --e; continue; } + + if (ShouldCheckConstantCond && + LoVal <= ConstantCondValue && + ConstantCondValue <= HiVal) + ShouldCheckConstantCond = false; + HiVals.push_back(HiVal); } @@ -779,18 +809,32 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, } } - // Check to see if switch is over an Enum and handles all of its - // values + // Complain if we have a constant condition and we didn't find a match. + if (!CaseListIsErroneous && ShouldCheckConstantCond) { + // TODO: it would be nice if we printed enums as enums, chars as + // chars, etc. + Diag(CondExpr->getExprLoc(), diag::warn_missing_case_for_condition) + << ConstantCondValue.toString(10) + << CondExpr->getSourceRange(); + } + + // Check to see if switch is over an Enum and handles all of its + // values. We don't need to do this if there's a default + // statement or if we have a constant condition. + // + // TODO: we might want to check whether case values are out of the + // enum even if we don't want to check whether all cases are handled. const EnumType* ET = CondTypeBeforePromotion->getAs(); // If switch has default case, then ignore it. - if (!CaseListIsErroneous && !TheDefaultStmt && ET) { + if (!CaseListIsErroneous && !TheDefaultStmt && !HasConstantCond && ET) { const EnumDecl *ED = ET->getDecl(); typedef llvm::SmallVector, 64> EnumValsTy; EnumValsTy EnumVals; - // Gather all enum values, set their type and sort them, allowing easier comparison - // with CaseVals. - for (EnumDecl::enumerator_iterator EDI = ED->enumerator_begin(); EDI != ED->enumerator_end(); EDI++) { + // Gather all enum values, set their type and sort them, + // allowing easier comparison with CaseVals. + for (EnumDecl::enumerator_iterator EDI = ED->enumerator_begin(); + EDI != ED->enumerator_end(); EDI++) { llvm::APSInt Val = (*EDI)->getInitVal(); if(Val.getBitWidth() < CondWidth) Val.extend(CondWidth); @@ -798,30 +842,36 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, EnumVals.push_back(std::make_pair(Val, (*EDI))); } std::stable_sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals); - EnumValsTy::iterator EIend = std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals); + EnumValsTy::iterator EIend = + std::unique(EnumVals.begin(), EnumVals.end(), EqEnumVals); // See which case values aren't in enum EnumValsTy::const_iterator EI = EnumVals.begin(); - for (CaseValsTy::const_iterator CI = CaseVals.begin(); CI != CaseVals.end(); CI++) { + for (CaseValsTy::const_iterator CI = CaseVals.begin(); + CI != CaseVals.end(); CI++) { while (EI != EIend && EI->first < CI->first) EI++; if (EI == EIend || EI->first > CI->first) - Diag(CI->second->getLHS()->getExprLoc(), diag::not_in_enum) << ED->getDeclName(); + Diag(CI->second->getLHS()->getExprLoc(), diag::warn_not_in_enum) + << ED->getDeclName(); } // See which of case ranges aren't in enum EI = EnumVals.begin(); - for (CaseRangesTy::const_iterator RI = CaseRanges.begin(); RI != CaseRanges.end() && EI != EIend; RI++) { + for (CaseRangesTy::const_iterator RI = CaseRanges.begin(); + RI != CaseRanges.end() && EI != EIend; RI++) { while (EI != EIend && EI->first < RI->first) EI++; if (EI == EIend || EI->first != RI->first) { - Diag(RI->second->getLHS()->getExprLoc(), diag::not_in_enum) << ED->getDeclName(); + Diag(RI->second->getLHS()->getExprLoc(), diag::warn_not_in_enum) + << ED->getDeclName(); } llvm::APSInt Hi = RI->second->getRHS()->EvaluateAsInt(Context); while (EI != EIend && EI->first < Hi) EI++; if (EI == EIend || EI->first != Hi) - Diag(RI->second->getRHS()->getExprLoc(), diag::not_in_enum) << ED->getDeclName(); + Diag(RI->second->getRHS()->getExprLoc(), diag::warn_not_in_enum) + << ED->getDeclName(); } //Check which enum vals aren't in switch CaseValsTy::const_iterator CI = CaseVals.begin(); @@ -844,7 +894,8 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, StmtArg Switch, } if (RI == CaseRanges.end() || EI->first < RI->first) - Diag(CondExpr->getExprLoc(), diag::warn_missing_cases) << EI->second->getDeclName(); + Diag(CondExpr->getExprLoc(), diag::warn_missing_cases) + << EI->second->getDeclName(); } } } diff --git a/test/Sema/switch.c b/test/Sema/switch.c index e63a1942bb..27ad06657e 100644 --- a/test/Sema/switch.c +++ b/test/Sema/switch.c @@ -24,36 +24,37 @@ void foo(int X) { void test3(void) { // empty switch; - switch (0); + switch (0); // expected-warning {{no case matching constant switch condition '0'}} } extern int g(); void test4() { - switch (1) { + int cond; + switch (cond) { case 0 && g(): case 1 || g(): break; } - switch(1) { + switch(cond) { case g(): // expected-error {{expression is not an integer constant expression}} case 0 ... g(): // expected-error {{expression is not an integer constant expression}} break; } - switch (1) { + switch (cond) { case 0 && g() ... 1 || g(): break; } - switch (1) { + switch (cond) { case g() && 0: // expected-error {{expression is not an integer constant expression}} // expected-note {{subexpression not valid in an integer constant expression}} break; } - switch (1) { + switch (cond) { case 0 ... g() || 1: // expected-error {{expression is not an integer constant expression}} // expected-note {{subexpression not valid in an integer constant expression}} break; } @@ -68,7 +69,7 @@ void test5(int z) { } void test6() { - const char ch = 'a'; + char ch = 'a'; switch(ch) { case 1234: // expected-warning {{overflow converting case value}} break; @@ -261,3 +262,18 @@ void f1(unsigned x) { default: break; } } + +void test15() { + int i = 0; + switch (1) { // expected-warning {{no case matching constant switch condition '1'}} + case 0: i = 0; break; + case 2: i++; break; + } +} + +void test16() { + const char c = '5'; + switch (c) { // expected-warning {{no case matching constant switch condition '53'}} + case '6': return; + } +} diff --git a/test/SemaCXX/constant-expression.cpp b/test/SemaCXX/constant-expression.cpp index a17dd58dab..1341036d83 100644 --- a/test/SemaCXX/constant-expression.cpp +++ b/test/SemaCXX/constant-expression.cpp @@ -57,8 +57,8 @@ template struct C { i10 = sizeof(Struct), i11 = true? 1 + cval * Struct::sval ^ itval / (int)1.5 - sizeof(Struct) : 0 ; - void f() { - switch(0) { + void f(int cond) { + switch(cond) { case 0 + 1: case 100 + eval: case 200 + cval: diff --git a/test/SemaCXX/i-c-e-cxx.cpp b/test/SemaCXX/i-c-e-cxx.cpp index e8275d463d..9672a420dc 100644 --- a/test/SemaCXX/i-c-e-cxx.cpp +++ b/test/SemaCXX/i-c-e-cxx.cpp @@ -17,7 +17,7 @@ void f() { int a() { const int t=t; // expected-note {{subexpression not valid}} - switch(1) { + switch(1) { // expected-warning {{no case matching constant switch condition '1'}} case t:; // expected-error {{not an integer constant expression}} } } diff --git a/test/SemaCXX/switch.cpp b/test/SemaCXX/switch.cpp index c256960af1..fc13630bbf 100644 --- a/test/SemaCXX/switch.cpp +++ b/test/SemaCXX/switch.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify %s +// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-enum %s void test() { bool x = true; @@ -40,3 +40,20 @@ void x3(C &c) { switch (c) { // expected-error{{incomplete class type}} } } + +namespace test3 { + enum En { A, B, C }; + template void foo() { + int x = 0, y = 5; + + switch (how) { //expected-warning {{no case matching constant switch condition '2'}} + case A: x *= y; break; + case B: x += y; break; + // No case for C, but it's okay because we have a constant condition. + } + } + + template void foo(); + template void foo(); + template void foo(); //expected-note {{in instantiation}} +}