From: Anders Carlsson Date: Sun, 22 Jul 2007 07:07:56 +0000 (+0000) Subject: Refactor switch analysis to make it possible to detect duplicate case values X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c1fcb7762673be706b0a40477d5e93411e918f93;p=clang Refactor switch analysis to make it possible to detect duplicate case values git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@40388 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/AST/StmtPrinter.cpp b/AST/StmtPrinter.cpp index 05d794c4c7..64e94f9048 100644 --- a/AST/StmtPrinter.cpp +++ b/AST/StmtPrinter.cpp @@ -218,6 +218,10 @@ void StmtPrinter::VisitSwitchStmt(SwitchStmt *Node) { } } +void StmtPrinter::VisitSwitchCase(SwitchCase*) { + assert(0 && "SwitchCase is an abstract class"); +} + void StmtPrinter::VisitWhileStmt(WhileStmt *Node) { Indent() << "while ("; PrintExpr(Node->getCond()); diff --git a/Parse/ParseStmt.cpp b/Parse/ParseStmt.cpp index 564b09722f..6784f166c9 100644 --- a/Parse/ParseStmt.cpp +++ b/Parse/ParseStmt.cpp @@ -475,14 +475,24 @@ Parser::StmtResult Parser::ParseSwitchStatement() { // Parse the condition. ExprResult Cond = ParseSimpleParenExpression(); + if (Cond.isInvalid) { + ExitScope(); + return true; + } + + StmtResult Switch = Actions.StartSwitchStmt(Cond.Val); + // Read the body statement. StmtResult Body = ParseStatement(); - ExitScope(); + if (Body.isInvalid) { + Body = Actions.ParseNullStmt(Tok.getLocation()); + // FIXME: Remove the case statement list from the Switch statement. + } - if (Cond.isInvalid || Body.isInvalid) return true; + ExitScope(); - return Actions.ParseSwitchStmt(SwitchLoc, Cond.Val, Body.Val); + return Actions.FinishSwitchStmt(SwitchLoc, Switch.Val, Body.Val); } /// ParseWhileStatement diff --git a/Sema/Sema.h b/Sema/Sema.h index b8a03bc096..39a3cc08f1 100644 --- a/Sema/Sema.h +++ b/Sema/Sema.h @@ -17,6 +17,7 @@ #include "clang/Parse/Action.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallVector.h" #include #include @@ -36,6 +37,7 @@ namespace clang { class IntegerLiteral; class ArrayType; class LabelStmt; + class SwitchStmt; /// Sema - This implements semantic analysis and AST building for C. class Sema : public Action { @@ -57,6 +59,8 @@ class Sema : public Action { /// it (which acts like the label decl in some ways). Forward referenced /// labels have a LabelStmt created for them with a null location & SubStmt. llvm::DenseMap LabelMap; + + llvm::SmallVector SwitchStack; public: Sema(Preprocessor &pp, ASTContext &ctxt, std::vector &prevInGroup); @@ -175,8 +179,9 @@ public: virtual StmtResult ParseIfStmt(SourceLocation IfLoc, ExprTy *CondVal, StmtTy *ThenVal, SourceLocation ElseLoc, StmtTy *ElseVal); - virtual StmtResult ParseSwitchStmt(SourceLocation SwitchLoc, ExprTy *Cond, - StmtTy *Body); + virtual StmtResult StartSwitchStmt(ExprTy *Cond); + virtual StmtResult FinishSwitchStmt(SourceLocation SwitchLoc, StmtTy *Switch, + ExprTy *Body); virtual StmtResult ParseWhileStmt(SourceLocation WhileLoc, ExprTy *Cond, StmtTy *Body); virtual StmtResult ParseDoStmt(SourceLocation DoLoc, StmtTy *Body, diff --git a/Sema/SemaStmt.cpp b/Sema/SemaStmt.cpp index 17e042c9f1..33a11344e7 100644 --- a/Sema/SemaStmt.cpp +++ b/Sema/SemaStmt.cpp @@ -18,6 +18,7 @@ #include "clang/Basic/Diagnostic.h" #include "clang/Basic/LangOptions.h" #include "clang/Lex/IdentifierTable.h" + using namespace clang; Sema::StmtResult Sema::ParseExprStmt(ExprTy *expr) { @@ -56,7 +57,7 @@ Sema::ParseCaseStmt(SourceLocation CaseLoc, ExprTy *lhsval, Stmt *SubStmt = static_cast(subStmt); Expr *LHSVal = ((Expr *)lhsval); assert((LHSVal != 0) && "missing expression in case statement"); - + SourceLocation ExpLoc; // C99 6.8.4.2p3: The expression shall be an integer constant. if (!LHSVal->isIntegerConstantExpr(Context, &ExpLoc)) { @@ -73,7 +74,13 @@ Sema::ParseCaseStmt(SourceLocation CaseLoc, ExprTy *lhsval, return SubStmt; } - return new CaseStmt(LHSVal, (Expr*)RHSVal, SubStmt); + CaseStmt *CS = new CaseStmt(LHSVal, RHSVal, SubStmt); + + assert(!SwitchStack.empty() && "missing push/pop in switch stack!"); + SwitchStmt *SS = SwitchStack.back(); + SS->addSwitchCase(CS); + + return CS; } Action::StmtResult @@ -87,15 +94,12 @@ Sema::ParseDefaultStmt(SourceLocation DefaultLoc, SourceLocation ColonLoc, return SubStmt; } - if (S->getDefaultStmt()) { - Diag(DefaultLoc, diag::err_multiple_default_labels_defined); - Diag(((DefaultStmt *)S->getDefaultStmt())->getDefaultLoc(), - diag::err_first_label); - return SubStmt; - } - DefaultStmt *DS = new DefaultStmt(DefaultLoc, SubStmt); - S->setDefaultStmt(DS); + + assert(!SwitchStack.empty() && "missing push/pop in switch stack!"); + SwitchStmt *SS = SwitchStack.back(); + SS->addSwitchCase(DS); + return DS; } @@ -145,16 +149,54 @@ Sema::ParseIfStmt(SourceLocation IfLoc, ExprTy *CondVal, } Action::StmtResult -Sema::ParseSwitchStmt(SourceLocation SwitchLoc, ExprTy *Cond, StmtTy *Body) { - Expr *condExpr = (Expr *)Cond; +Sema::StartSwitchStmt(ExprTy *Cond) { + SwitchStmt *SS = new SwitchStmt((Expr*)Cond); + SwitchStack.push_back(SS); + return SS; +} + +Action::StmtResult +Sema::FinishSwitchStmt(SourceLocation SwitchLoc, StmtTy *Switch, ExprTy *Body) { + Stmt *BodyStmt = (Stmt*)Body; + + SwitchStmt *SS = SwitchStack.back(); + assert(SS == (SwitchStmt*)Switch && "switch stack missing push/pop!"); + + SS->setBody(BodyStmt); + SwitchStack.pop_back(); + Expr *condExpr = SS->getCond(); QualType condType = condExpr->getType(); - if (!condType->isIntegerType()) // C99 6.8.4.2p1 - return Diag(SwitchLoc, diag::err_typecheck_statement_requires_integer, - condType.getAsString(), condExpr->getSourceRange()); + if (!condType->isIntegerType()) { // C99 6.8.4.2p1 + Diag(SwitchLoc, diag::err_typecheck_statement_requires_integer, + condType.getAsString(), condExpr->getSourceRange()); + return false; + } + + DefaultStmt *CurDefaultStmt = 0; + + // FIXME: The list of cases is backwards and needs to be reversed. + for (SwitchCase *SC = SS->getSwitchCaseList(); SC; + SC = SC->getNextSwitchCase()) { + if (DefaultStmt *DS = dyn_cast(SC)) { + if (CurDefaultStmt) { + Diag(DS->getDefaultLoc(), + diag::err_multiple_default_labels_defined); + Diag(CurDefaultStmt->getDefaultLoc(), + diag::err_first_label); + + // FIXME: Remove the default statement from the switch block + // so that we'll return a valid AST. + } else { + CurDefaultStmt = DS; + } + + // FIXME: Check that case values are unique here. + } + } - return new SwitchStmt((Expr*)Cond, (Stmt*)Body); + return SS; } Action::StmtResult diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h index 3de6268cfd..4fcf7aecb8 100644 --- a/include/clang/AST/Stmt.h +++ b/include/clang/AST/Stmt.h @@ -124,15 +124,37 @@ public: static bool classof(const CompoundStmt *) { return true; } }; -class CaseStmt : public Stmt { +// SwitchCase is the base class for CaseStmt and DefaultStmt, +class SwitchCase : public Stmt { + // A pointer to the following CaseStmt or DefaultStmt class, + // used by SwitchStmt. + SwitchCase *NextSwitchCase; +protected: + SwitchCase(StmtClass SC) : Stmt(SC), NextSwitchCase(0) {} + +public: + const SwitchCase *getNextSwitchCase() const { return NextSwitchCase; } + + SwitchCase *getNextSwitchCase() { return NextSwitchCase; } + + void setNextSwitchCase(SwitchCase *SC) { NextSwitchCase = SC; } + + static bool classof(const Stmt *T) { + return T->getStmtClass() == CaseStmtClass || + T->getStmtClass() == DefaultStmtClass; + } + static bool classof(const SwitchCase *) { return true; } + + virtual void visit(StmtVisitor &Visitor) = 0; +}; + +class CaseStmt : public SwitchCase { Expr *LHSVal; Expr *RHSVal; // Non-null for GNU "case 1 ... 4" extension Stmt *SubStmt; - SwitchStmt *Switch; public: CaseStmt(Expr *lhs, Expr *rhs, Stmt *substmt) - : Stmt(CaseStmtClass), LHSVal(lhs), RHSVal(rhs), SubStmt(substmt), - Switch(0) {} + : SwitchCase(CaseStmtClass), LHSVal(lhs), RHSVal(rhs), SubStmt(substmt) {} Expr *getLHS() { return LHSVal; } Expr *getRHS() { return RHSVal; } @@ -145,11 +167,11 @@ public: static bool classof(const CaseStmt *) { return true; } }; -class DefaultStmt : public Stmt { +class DefaultStmt : public SwitchCase { SourceLocation DefaultLoc; Stmt *SubStmt; public: - DefaultStmt(SourceLocation DL, Stmt *substmt) : Stmt(DefaultStmtClass), + DefaultStmt(SourceLocation DL, Stmt *substmt) : SwitchCase(DefaultStmtClass), DefaultLoc(DL), SubStmt(substmt) {} SourceLocation getDefaultLoc() const { return DefaultLoc; } @@ -216,12 +238,29 @@ public: class SwitchStmt : public Stmt { Expr *Cond; Stmt *Body; + + // This points to a linked list of case and default statements. + SwitchCase *FirstCase; public: - SwitchStmt(Expr *cond, Stmt *body) - : Stmt(SwitchStmtClass), Cond(cond), Body(body) {} + SwitchStmt(Expr *cond) + : Stmt(SwitchStmtClass), Cond(cond), Body(0), FirstCase(0) {} + const Expr *getCond() const { return Cond; } + const Stmt *getBody() const { return Body; } + const SwitchCase *getSwitchCaseList() const { return FirstCase; } + Expr *getCond() { return Cond; } Stmt *getBody() { return Body; } + SwitchCase *getSwitchCaseList() { return FirstCase; } + + void setBody(Stmt *S) { Body = S; } + + void addSwitchCase(SwitchCase *SC) { + if (FirstCase) + SC->setNextSwitchCase(FirstCase); + + FirstCase = SC; + } virtual void visit(StmtVisitor &Visitor); static bool classof(const Stmt *T) { diff --git a/include/clang/AST/StmtNodes.def b/include/clang/AST/StmtNodes.def index 6f85057686..b4c220c0e2 100644 --- a/include/clang/AST/StmtNodes.def +++ b/include/clang/AST/StmtNodes.def @@ -25,8 +25,8 @@ FIRST_STMT(1) STMT( 1, NullStmt , Stmt) STMT( 2, CompoundStmt , Stmt) -STMT( 3, CaseStmt , Stmt) -STMT( 4, DefaultStmt , Stmt) +STMT( 3, CaseStmt , SwitchCase) +STMT( 4, DefaultStmt , SwitchCase) STMT( 5, LabelStmt , Stmt) STMT( 6, IfStmt , Stmt) STMT( 7, SwitchStmt , Stmt) @@ -39,7 +39,8 @@ STMT(13, ContinueStmt , Stmt) STMT(14, BreakStmt , Stmt) STMT(15, ReturnStmt , Stmt) STMT(16, DeclStmt , Stmt) -LAST_STMT(16) +STMT(17, SwitchCase , Stmt) +LAST_STMT(17) FIRST_EXPR(31) // Expressions. diff --git a/include/clang/Parse/Action.h b/include/clang/Parse/Action.h index c80effe156..4cb49b2ac6 100644 --- a/include/clang/Parse/Action.h +++ b/include/clang/Parse/Action.h @@ -220,10 +220,15 @@ public: return 0; } - virtual StmtResult ParseSwitchStmt(SourceLocation SwitchLoc, ExprTy *Cond, - StmtTy *Body) { + virtual StmtResult StartSwitchStmt(ExprTy *Cond) { return 0; } + + virtual StmtResult FinishSwitchStmt(SourceLocation SwitchLoc, StmtTy *Switch, + ExprTy *Body) { + return 0; + } + virtual StmtResult ParseWhileStmt(SourceLocation WhileLoc, ExprTy *Cond, StmtTy *Body) { return 0; diff --git a/include/clang/Parse/Scope.h b/include/clang/Parse/Scope.h index b02f082934..7f721f803d 100644 --- a/include/clang/Parse/Scope.h +++ b/include/clang/Parse/Scope.h @@ -79,9 +79,6 @@ private: typedef llvm::SmallPtrSet DeclSetTy; DeclSetTy DeclsInScope; - /// DefaultStmt - when parsing the body of a switch statement, this keeps - /// track of the statement with the default label. - Action::StmtTy *DefaultStmt; public: Scope(Scope *Parent, unsigned ScopeFlags) { Init(Parent, ScopeFlags); @@ -118,9 +115,6 @@ public: return DeclsInScope.count(D) != 0; } - void setDefaultStmt(Action::StmtTy *S) { DefaultStmt = S; } - Action::StmtTy *getDefaultStmt() const { return DefaultStmt; } - /// Init - This is used by the parser to implement scope caching. /// void Init(Scope *Parent, unsigned ScopeFlags) { @@ -138,8 +132,6 @@ public: FnParent = BreakParent = ContinueParent = 0; } - DefaultStmt = 0; - // If this scope is a function or contains breaks/continues, remember it. if (Flags & FnScope) FnParent = this; if (Flags & BreakScope) BreakParent = this; diff --git a/test/Sema/switch-duplicate-defaults.c b/test/Sema/switch-duplicate-defaults.c new file mode 100644 index 0000000000..31d46a1bbc --- /dev/null +++ b/test/Sema/switch-duplicate-defaults.c @@ -0,0 +1,10 @@ +// RUN: clang -parse-ast-check %s + +void f (int z) { + switch(z) { + default: // expected-error {{first label is here}} + default: // expected-error {{multiple default labels in one switch}} + break; + } +} +