[cfe-commits] r147202 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticParseKinds.td include/clang/Parse/Parser.h lib/Parse/ParseStmt.cpp test/Parser/warn-dangling-else.cpp
David Blaikie
dblaikie at gmail.com
Mon Jan 16 19:13:14 PST 2012
On Mon, Jan 16, 2012 at 7:03 PM, Chad Rosier <mcrosier at apple.com> wrote:
> Hi Nico,
> I get a warning with the following test case.
>
> int foo(int a, int b, int c) {
> int rval = 0;
> if (a != b)
> if (b == c) {
> rval = 0;
> } else {
> rval = 1;
> }
> return rval;
> }
>
> mcrosier$ clang -O3 t.c -Wall -c -o /dev/null
> t.c:6:11: warning: add explicit braces to avoid dangling else [-Wdangling-else]
> } else {
> ^
> 1 warning generated.
>
> Is this expected behavior? This code seems rather explicit to me.
(bystander's impression/opinion) - judging by the referenced article,
I'd say this is probably expected behavior because it is, essentially,
ambiguous. No heuristics are being used here - it's simply "if this
else could be attached to more than one if, it's ambiguous & we'll
warn" - in this case it's still possible for you to have written:
if (a != b)
if (b == c) {
rval = 0;
}
else {
rval = 1;
}
It's possible we could add some heuristics (though that risks missing
cases pretty easily I'd imagine) or possibly whitespace checking to
see whether the user seems to have indicated their understanding of
the precedence rules (though see the various minor false positives
with that sort of approach from the recent spurious semicolon ("while
(cond); { code }") warning work that took a similar approach - not
totally unacceptable, but an extra variable to consider)
- David
> On Dec 22, 2011, at 3:26 PM, Nico Weber wrote:
>
>> Author: nico
>> Date: Thu Dec 22 17:26:17 2011
>> New Revision: 147202
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=147202&view=rev
>> Log:
>> Add -Wdangling-else.
>>
>> This works like described in http://drdobbs.com/blogs/cpp/231602010
>> Fixes http://llvm.org/PR11609
>>
>>
>> Added:
>> cfe/trunk/test/Parser/warn-dangling-else.cpp
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> cfe/trunk/include/clang/Parse/Parser.h
>> cfe/trunk/lib/Parse/ParseStmt.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=147202&r1=147201&r2=147202&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Dec 22 17:26:17 2011
>> @@ -85,6 +85,7 @@
>> 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 @@
>> // 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
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=147202&r1=147201&r2=147202&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Thu Dec 22 17:26:17 2011
>> @@ -387,6 +387,9 @@
>> "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<
>>
>> Modified: cfe/trunk/include/clang/Parse/Parser.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=147202&r1=147201&r2=147202&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Parse/Parser.h (original)
>> +++ cfe/trunk/include/clang/Parse/Parser.h Thu Dec 22 17:26:17 2011
>> @@ -1466,9 +1466,9 @@
>> 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 @@
>> //===--------------------------------------------------------------------===//
>> // 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 @@
>> 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);
>>
>> Modified: cfe/trunk/lib/Parse/ParseStmt.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=147202&r1=147201&r2=147202&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Parse/ParseStmt.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseStmt.cpp Thu Dec 22 17:26:17 2011
>> @@ -76,7 +76,8 @@
>> /// [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 @@
>> }
>>
>> 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 @@
>> /// [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 @@
>>
>> // 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 ElseStmt;
>>
>> if (Tok.is(tok::kw_else)) {
>> + if (TrailingElseLoc)
>> + *TrailingElseLoc = Tok.getLocation();
>> +
>> ElseLoc = ConsumeToken();
>> ElseStmtLoc = Tok.getLocation();
>>
>> @@ -967,6 +974,8 @@
>> Actions.CodeCompleteAfterIf(getCurScope());
>> cutOffParsing();
>> return StmtError();
>> + } else if (InnerStatementTrailingElseLoc.isValid()) {
>> + Diag(InnerStatementTrailingElseLoc, diag::warn_dangling_else);
>> }
>>
>> IfScope.Exit();
>> @@ -1000,7 +1009,8 @@
>> /// 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 @@
>> 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 @@
>> /// 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 @@
>> 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 @@
>> /// [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 @@
>> 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();
>>
>> Added: cfe/trunk/test/Parser/warn-dangling-else.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/warn-dangling-else.cpp?rev=147202&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Parser/warn-dangling-else.cpp (added)
>> +++ cfe/trunk/test/Parser/warn-dangling-else.cpp Thu Dec 22 17:26:17 2011
>> @@ -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
>> +}
>> +
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list