From 5cb94a78202ccb1007df0be86884297761f4a53a Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 22 Dec 2011 23:26:17 +0000 Subject: [PATCH] Add -Wdangling-else. This works like described in http://drdobbs.com/blogs/cpp/231602010 Fixes http://llvm.org/PR11609 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@147202 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 5 +- include/clang/Basic/DiagnosticParseKinds.td | 3 ++ include/clang/Parse/Parser.h | 25 ++++++---- lib/Parse/ParseStmt.cpp | 38 +++++++++----- test/Parser/warn-dangling-else.cpp | 55 +++++++++++++++++++++ 5 files changed, 101 insertions(+), 25 deletions(-) create mode 100644 test/Parser/warn-dangling-else.cpp diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 554961b7b2..0f3d67b53e 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -85,6 +85,7 @@ def GlobalConstructors : DiagGroup<"global-constructors">; def : DiagGroup<"idiomatic-parentheses">; def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">; def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; +def DanglingElse: DiagGroup<"dangling-else">; def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">; def : DiagGroup<"import">; def IncompatiblePointerTypes : DiagGroup<"incompatible-pointer-types">; @@ -322,8 +323,8 @@ def Most : DiagGroup<"most", [ // Thread Safety warnings def ThreadSafety : DiagGroup<"thread-safety">; -// -Wall is -Wmost -Wparentheses -Wtop-level-comparison -def : DiagGroup<"all", [Most, Parentheses]>; +// -Wall is -Wmost -Wparentheses -Wdangling-else +def : DiagGroup<"all", [DanglingElse, Most, Parentheses]>; // Aliases. def : DiagGroup<"", [Extra]>; // -W = -Wextra diff --git a/include/clang/Basic/DiagnosticParseKinds.td b/include/clang/Basic/DiagnosticParseKinds.td index 74c4418ce6..8c3d43b249 100644 --- a/include/clang/Basic/DiagnosticParseKinds.td +++ b/include/clang/Basic/DiagnosticParseKinds.td @@ -387,6 +387,9 @@ def err_expected_equal_after_declarator : Error< "expected '=' after declarator">; def warn_parens_disambiguated_as_function_decl : Warning< "parentheses were disambiguated as a function declarator">; +def warn_dangling_else : Warning< + "add explicit braces to avoid dangling else">, + InGroup; def err_expected_member_or_base_name : Error< "expected class member or base class name">; def err_expected_lbrace_after_base_specifiers : Error< diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h index e4318375af..293c46d1e2 100644 --- a/include/clang/Parse/Parser.h +++ b/include/clang/Parse/Parser.h @@ -1466,9 +1466,9 @@ private: bool isSimpleObjCMessageExpression(); ExprResult ParseObjCMessageExpression(); ExprResult ParseObjCMessageExpressionBody(SourceLocation LBracloc, - SourceLocation SuperLoc, - ParsedType ReceiverType, - ExprArg ReceiverExpr); + SourceLocation SuperLoc, + ParsedType ReceiverType, + ExprArg ReceiverExpr); ExprResult ParseAssignmentExprWithObjCMessageExprStart( SourceLocation LBracloc, SourceLocation SuperLoc, ParsedType ReceiverType, ExprArg ReceiverExpr); @@ -1477,12 +1477,13 @@ private: //===--------------------------------------------------------------------===// // C99 6.8: Statements and Blocks. - StmtResult ParseStatement() { + StmtResult ParseStatement(SourceLocation *TrailingElseLoc = NULL) { StmtVector Stmts(Actions); - return ParseStatementOrDeclaration(Stmts, true); + return ParseStatementOrDeclaration(Stmts, true, TrailingElseLoc); } StmtResult ParseStatementOrDeclaration(StmtVector& Stmts, - bool OnlyStatement = false); + bool OnlyStatement, + SourceLocation *TrailingElseLoc = NULL); StmtResult ParseExprStatement(ParsedAttributes &Attrs); StmtResult ParseLabeledStatement(ParsedAttributes &Attr); StmtResult ParseCaseStatement(ParsedAttributes &Attr, @@ -1499,11 +1500,15 @@ private: Decl *&DeclResult, SourceLocation Loc, bool ConvertToBoolean); - StmtResult ParseIfStatement(ParsedAttributes &Attr); - StmtResult ParseSwitchStatement(ParsedAttributes &Attr); - StmtResult ParseWhileStatement(ParsedAttributes &Attr); + StmtResult ParseIfStatement(ParsedAttributes &Attr, + SourceLocation *TrailingElseLoc); + StmtResult ParseSwitchStatement(ParsedAttributes &Attr, + SourceLocation *TrailingElseLoc); + StmtResult ParseWhileStatement(ParsedAttributes &Attr, + SourceLocation *TrailingElseLoc); StmtResult ParseDoStatement(ParsedAttributes &Attr); - StmtResult ParseForStatement(ParsedAttributes &Attr); + StmtResult ParseForStatement(ParsedAttributes &Attr, + SourceLocation *TrailingElseLoc); StmtResult ParseGotoStatement(ParsedAttributes &Attr); StmtResult ParseContinueStatement(ParsedAttributes &Attr); StmtResult ParseBreakStatement(ParsedAttributes &Attr); diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp index 13b3bdaf74..fadf34fc4f 100644 --- a/lib/Parse/ParseStmt.cpp +++ b/lib/Parse/ParseStmt.cpp @@ -76,7 +76,8 @@ using namespace clang; /// [OBC] '@' 'throw' ';' /// StmtResult -Parser::ParseStatementOrDeclaration(StmtVector &Stmts, bool OnlyStatement) { +Parser::ParseStatementOrDeclaration(StmtVector &Stmts, bool OnlyStatement, + SourceLocation *TrailingElseLoc) { const char *SemiError = 0; StmtResult Res; @@ -234,18 +235,18 @@ Retry: } case tok::kw_if: // C99 6.8.4.1: if-statement - return ParseIfStatement(attrs); + return ParseIfStatement(attrs, TrailingElseLoc); case tok::kw_switch: // C99 6.8.4.2: switch-statement - return ParseSwitchStatement(attrs); + return ParseSwitchStatement(attrs, TrailingElseLoc); case tok::kw_while: // C99 6.8.5.1: while-statement - return ParseWhileStatement(attrs); + return ParseWhileStatement(attrs, TrailingElseLoc); case tok::kw_do: // C99 6.8.5.2: do-statement Res = ParseDoStatement(attrs); SemiError = "do/while"; break; case tok::kw_for: // C99 6.8.5.3: for-statement - return ParseForStatement(attrs); + return ParseForStatement(attrs, TrailingElseLoc); case tok::kw_goto: // C99 6.8.6.1: goto-statement Res = ParseGotoStatement(attrs); @@ -874,7 +875,8 @@ bool Parser::ParseParenExprOrCondition(ExprResult &ExprResult, /// [C++] 'if' '(' condition ')' statement /// [C++] 'if' '(' condition ')' statement 'else' statement /// -StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs) { +StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs, + SourceLocation *TrailingElseLoc) { // FIXME: Use attributes? assert(Tok.is(tok::kw_if) && "Not an if stmt!"); @@ -933,7 +935,9 @@ StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs) { // Read the 'then' stmt. SourceLocation ThenStmtLoc = Tok.getLocation(); - StmtResult ThenStmt(ParseStatement()); + + SourceLocation InnerStatementTrailingElseLoc; + StmtResult ThenStmt(ParseStatement(&InnerStatementTrailingElseLoc)); // Pop the 'if' scope if needed. InnerScope.Exit(); @@ -944,6 +948,9 @@ StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs) { StmtResult ElseStmt; if (Tok.is(tok::kw_else)) { + if (TrailingElseLoc) + *TrailingElseLoc = Tok.getLocation(); + ElseLoc = ConsumeToken(); ElseStmtLoc = Tok.getLocation(); @@ -967,6 +974,8 @@ StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs) { Actions.CodeCompleteAfterIf(getCurScope()); cutOffParsing(); return StmtError(); + } else if (InnerStatementTrailingElseLoc.isValid()) { + Diag(InnerStatementTrailingElseLoc, diag::warn_dangling_else); } IfScope.Exit(); @@ -1000,7 +1009,8 @@ StmtResult Parser::ParseIfStatement(ParsedAttributes &attrs) { /// switch-statement: /// 'switch' '(' expression ')' statement /// [C++] 'switch' '(' condition ')' statement -StmtResult Parser::ParseSwitchStatement(ParsedAttributes &attrs) { +StmtResult Parser::ParseSwitchStatement(ParsedAttributes &attrs, + SourceLocation *TrailingElseLoc) { // FIXME: Use attributes? assert(Tok.is(tok::kw_switch) && "Not a switch stmt!"); @@ -1068,7 +1078,7 @@ StmtResult Parser::ParseSwitchStatement(ParsedAttributes &attrs) { C99orCXX && Tok.isNot(tok::l_brace)); // Read the body statement. - StmtResult Body(ParseStatement()); + StmtResult Body(ParseStatement(TrailingElseLoc)); // Pop the scopes. InnerScope.Exit(); @@ -1085,7 +1095,8 @@ StmtResult Parser::ParseSwitchStatement(ParsedAttributes &attrs) { /// while-statement: [C99 6.8.5.1] /// 'while' '(' expression ')' statement /// [C++] 'while' '(' condition ')' statement -StmtResult Parser::ParseWhileStatement(ParsedAttributes &attrs) { +StmtResult Parser::ParseWhileStatement(ParsedAttributes &attrs, + SourceLocation *TrailingElseLoc) { // FIXME: Use attributes? assert(Tok.is(tok::kw_while) && "Not a while stmt!"); @@ -1143,7 +1154,7 @@ StmtResult Parser::ParseWhileStatement(ParsedAttributes &attrs) { C99orCXX && Tok.isNot(tok::l_brace)); // Read the body statement. - StmtResult Body(ParseStatement()); + StmtResult Body(ParseStatement(TrailingElseLoc)); // Pop the body scope if needed. InnerScope.Exit(); @@ -1242,7 +1253,8 @@ StmtResult Parser::ParseDoStatement(ParsedAttributes &attrs) { /// [C++0x] for-range-initializer: /// [C++0x] expression /// [C++0x] braced-init-list [TODO] -StmtResult Parser::ParseForStatement(ParsedAttributes &attrs) { +StmtResult Parser::ParseForStatement(ParsedAttributes &attrs, + SourceLocation *TrailingElseLoc) { // FIXME: Use attributes? assert(Tok.is(tok::kw_for) && "Not a for stmt!"); @@ -1467,7 +1479,7 @@ StmtResult Parser::ParseForStatement(ParsedAttributes &attrs) { C99orCXXorObjC && Tok.isNot(tok::l_brace)); // Read the body statement. - StmtResult Body(ParseStatement()); + StmtResult Body(ParseStatement(TrailingElseLoc)); // Pop the body scope if needed. InnerScope.Exit(); diff --git a/test/Parser/warn-dangling-else.cpp b/test/Parser/warn-dangling-else.cpp new file mode 100644 index 0000000000..e91af9814f --- /dev/null +++ b/test/Parser/warn-dangling-else.cpp @@ -0,0 +1,55 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wdangling-else %s + +void f(int a, int b, int c, int d, int e) { + + // should warn + { if (a) if (b) d++; else e++; } // expected-warning {{add explicit braces to avoid dangling else}} + { if (a) while (b) if (c) d++; else e++; } // expected-warning {{add explicit braces to avoid dangling else}} + { if (a) switch (b) if (c) d++; else e++; } // expected-warning {{add explicit braces to avoid dangling else}} + { if (a) for (;;) if (c) d++; else e++; } // expected-warning {{add explicit braces to avoid dangling else}} + { if (a) if (b) if (d) d++; else e++; else d--; } // expected-warning {{add explicit braces to avoid dangling else}} + + if (a) + if (b) { + d++; + } else e++; // expected-warning {{add explicit braces to avoid dangling else}} + + // shouldn't + { if (a) if (b) d++; } + { if (a) if (b) if (c) d++; } + { if (a) if (b) d++; else e++; else d--; } + { if (a) if (b) if (d) d++; else e++; else d--; else e--; } + { if (a) do if (b) d++; else e++; while (c); } + + if (a) { + if (b) d++; + else e++; + } + + if (a) { + if (b) d++; + } else e++; +} + +// Somewhat more elaborate case that shouldn't warn. +class A { + public: + void operator<<(const char* s) {} +}; + +void HandleDisabledThing() {} +A GetThing() { return A(); } + +#define FOO(X) \ + switch (0) default: \ + if (!(X)) \ + HandleDisabledThing(); \ + else \ + GetThing() + +void f(bool cond) { + int x = 0; + if (cond) + FOO(x) << "hello"; // no warning +} + -- 2.40.0