From caaf29a08761b14fbe42a29080c22dd6961056d1 Mon Sep 17 00:00:00 2001 From: Douglas Gregor Date: Wed, 10 Dec 2008 23:01:14 +0000 Subject: [PATCH] Added a warning when referencing an if's condition variable in the "else" clause, e.g., if (int X = foo()) { } else { if (X) { // warning: X is always zero in this context } } Fixes rdar://6425550 and lets me think about something other than DeclContext. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@60858 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 22 ++++++++++++++++-- include/clang/Basic/DiagnosticKinds.def | 4 ++++ include/clang/Parse/Scope.h | 24 ++++++++++++++++++++ lib/Parse/ParseStmt.cpp | 3 +++ lib/Sema/SemaExpr.cpp | 21 +++++++++++++++++ lib/Sema/SemaExprCXX.cpp | 4 ++++ test/SemaCXX/warn-for-var-in-else.cpp | 30 +++++++++++++++++++++++++ 7 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 test/SemaCXX/warn-for-var-in-else.cpp diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index b4a1bf83da..8fbb3f00e9 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -331,7 +331,11 @@ private: unsigned SClass : 3; bool ThreadSpecified : 1; bool HasCXXDirectInit : 1; - + + /// DeclaredInCondition - Whether this variable was declared in a + /// condition, e.g., if (int x = foo()) { ... }. + bool DeclaredInCondition : 1; + // Move to DeclGroup when it is implemented. SourceLocation TypeSpecStartLoc; friend class StmtIteratorBase; @@ -341,7 +345,9 @@ protected: SourceLocation TSSL = SourceLocation()) : ValueDecl(DK, DC, L, Id, T, PrevDecl), Init(0), ThreadSpecified(false), HasCXXDirectInit(false), - TypeSpecStartLoc(TSSL) { SClass = SC; } + DeclaredInCondition(false), TypeSpecStartLoc(TSSL) { + SClass = SC; + } public: static VarDecl *Create(ASTContext &C, DeclContext *DC, SourceLocation L, IdentifierInfo *Id, @@ -373,6 +379,18 @@ public: return HasCXXDirectInit; } + /// isDeclaredInCondition - Whether this variable was declared as + /// part of a condition in an if/switch/while statement, e.g., + /// @code + /// if (int x = foo()) { ... } + /// @endcode + bool isDeclaredInCondition() const { + return DeclaredInCondition; + } + void setDeclaredInCondition(bool InCondition) { + DeclaredInCondition = InCondition; + } + /// hasLocalStorage - Returns true if a variable with function scope /// is a non-static local variable. bool hasLocalStorage() const { diff --git a/include/clang/Basic/DiagnosticKinds.def b/include/clang/Basic/DiagnosticKinds.def index 20337f0aed..bba58791d1 100644 --- a/include/clang/Basic/DiagnosticKinds.def +++ b/include/clang/Basic/DiagnosticKinds.def @@ -1294,6 +1294,10 @@ DIAG(err_invalid_declarator_in_function, ERROR, DIAG(err_not_tag_in_scope, ERROR, "%0 does not name a tag member in the specified scope") +DIAG(warn_value_always_zero, WARNING, + "%0 is always zero in this context") +DIAG(warn_value_always_false, WARNING, + "%0 is always false in this context") // assignment related diagnostics (also for argument passing, returning, etc). // FIXME: %2 is an english string here. diff --git a/include/clang/Parse/Scope.h b/include/clang/Parse/Scope.h index 5695fc0be1..f8815ffd4f 100644 --- a/include/clang/Parse/Scope.h +++ b/include/clang/Parse/Scope.h @@ -75,6 +75,10 @@ private: /// interrelates with other control flow statements. unsigned Flags : 8; + /// WithinElse - Whether this scope is part of the "else" branch in + /// its parent ControlScope. + bool WithinElse : 1; + /// FnParent - If this scope has a parent scope that is a function body, this /// pointer is non-null and points to it. This is used for label processing. Scope *FnParent; @@ -84,6 +88,11 @@ private: /// there is no containing break/continue scope. Scope *BreakParent, *ContinueParent; + /// ControlParent - This is a direct link to the immediately + /// preceeding ControlParent if this scope is not one, or null if + /// there is no containing control scope. + Scope *ControlParent; + /// BlockParent - This is a direct link to the immediately containing /// BlockScope if this scope is not one, or null if there is none. Scope *BlockParent; @@ -151,6 +160,9 @@ public: return const_cast(this)->getBreakParent(); } + Scope *getControlParent() { return ControlParent; } + const Scope *getControlParent() const { return ControlParent; } + Scope *getBlockParent() { return BlockParent; } const Scope *getBlockParent() const { return BlockParent; } @@ -193,6 +205,12 @@ public: return getFlags() & Scope::TemplateParamScope; } + /// isWithinElse - Whether we are within the "else" of the + /// ControlParent (if any). + bool isWithinElse() const { return WithinElse; } + + void setWithinElse(bool WE) { WithinElse = WE; } + /// Init - This is used by the parser to implement scope caching. /// void Init(Scope *Parent, unsigned ScopeFlags) { @@ -204,17 +222,23 @@ public: FnParent = AnyParent->FnParent; BreakParent = AnyParent->BreakParent; ContinueParent = AnyParent->ContinueParent; + ControlParent = AnyParent->ControlParent; BlockParent = AnyParent->BlockParent; TemplateParamParent = AnyParent->TemplateParamParent; + WithinElse = AnyParent->WithinElse; + } else { FnParent = BreakParent = ContinueParent = BlockParent = 0; + ControlParent = 0; TemplateParamParent = 0; + WithinElse = false; } // If this scope is a function or contains breaks/continues, remember it. if (Flags & FnScope) FnParent = this; if (Flags & BreakScope) BreakParent = this; if (Flags & ContinueScope) ContinueParent = this; + if (Flags & ControlScope) ControlParent = this; if (Flags & BlockScope) BlockParent = this; if (Flags & TemplateParamScope) TemplateParamParent = this; DeclsInScope.clear(); diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp index 8b5277bc36..cb35740465 100644 --- a/lib/Parse/ParseStmt.cpp +++ b/lib/Parse/ParseStmt.cpp @@ -509,8 +509,11 @@ Parser::StmtResult Parser::ParseIfStatement() { ParseScope InnerScope(this, Scope::DeclScope, C99orCXX && Tok.isNot(tok::l_brace)); + bool WithinElse = CurScope->isWithinElse(); + CurScope->setWithinElse(true); ElseStmtLoc = Tok.getLocation(); ElseStmt = ParseStatement(); + CurScope->setWithinElse(WithinElse); // Pop the 'else' scope if needed. InnerScope.Exit(); diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 8cbbca3fbf..3870f0736e 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -483,6 +483,27 @@ Sema::ExprResult Sema::ActOnDeclarationNameExpr(Scope *S, SourceLocation Loc, // check if referencing an identifier with __attribute__((deprecated)). if (VD->getAttr()) Diag(Loc, diag::warn_deprecated) << VD->getDeclName(); + + if (VarDecl *Var = dyn_cast(VD)) { + if (Var->isDeclaredInCondition() && Var->getType()->isScalarType()) { + Scope *CheckS = S; + while (CheckS) { + if (CheckS->isWithinElse() && + CheckS->getControlParent()->isDeclScope(Var)) { + if (Var->getType()->isBooleanType()) + Diag(Loc, diag::warn_value_always_false) << Var->getDeclName(); + else + Diag(Loc, diag::warn_value_always_zero) << Var->getDeclName(); + break; + } + + // Move up one more control parent to check again. + CheckS = CheckS->getControlParent(); + if (CheckS) + CheckS = CheckS->getParent(); + } + } + } // Only create DeclRefExpr's for valid Decl's. if (VD->isInvalidDecl()) diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index c4cab3316b..27b0ba34d1 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -648,6 +648,10 @@ Sema::ActOnCXXConditionDeclarationExpr(Scope *S, SourceLocation StartLoc, return true; AddInitializerToDecl(Dcl, AssignExprVal); + // Mark this variable as one that is declared within a conditional. + if (VarDecl *VD = dyn_cast((Decl *)Dcl)) + VD->setDeclaredInCondition(true); + return new CXXConditionDeclExpr(StartLoc, EqualLoc, cast(static_cast(Dcl))); } diff --git a/test/SemaCXX/warn-for-var-in-else.cpp b/test/SemaCXX/warn-for-var-in-else.cpp new file mode 100644 index 0000000000..dfbf35d17b --- /dev/null +++ b/test/SemaCXX/warn-for-var-in-else.cpp @@ -0,0 +1,30 @@ +// RUN: clang -fsyntax-only -verify %s +// rdar://6425550 +int bar(); +void do_something(int); + +int foo() { + if (int X = bar()) { + return X; + } else { + do_something(X); // expected-warning{{'X' is always zero in this context}} + } +} + +bool foo2() { + if (bool B = bar()) { + if (int Y = bar()) { + return B; + } else { + do_something(Y); // expected-warning{{'Y' is always zero in this context}} + return B; + } + } else { + if (bool B2 = B) { // expected-warning{{'B' is always false in this context}} + do_something(B); // expected-warning{{'B' is always false in this context}} + } else if (B2) { // expected-warning{{'B2' is always false in this context}} + do_something(B); // expected-warning{{'B' is always false in this context}} + } + return B; // expected-warning{{'B' is always false in this context}} + } +} -- 2.40.0