Add -Wdangling-else.

This works like described in  http://drdobbs.com/blogs/cpp/231602010
Fixes http://llvm.org/PR11609

llvm-svn: 147202

clang/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

clang/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<DanglingElse>;
 def err_expected_member_or_base_name : Error<
   "expected class member or base class name">;
 def err_expected_lbrace_after_base_specifiers : Error<

clang/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);

clang/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();

clang/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
+}
+