Another round of changes. I believe I addressed all the issues raised.
On Wed, Apr 20, 2011 at 7:35 AM, Douglas Gregor <[email protected]> wrote:
>
> On Apr 19, 2011, at 5:19 PM, Richard Trieu wrote:
>
> I have reworked the program flow. Instead of tentative parsing, the
> already parsed expression is reused within the case statement parsing
> following colon detection.
>
>
> I like this much better! A few more comments:
>
> Index: include/clang/Sema/Scope.h
> ===================================================================
> --- include/clang/Sema/Scope.h (revision 129825)
> +++ include/clang/Sema/Scope.h (working copy)
> @@ -75,7 +75,10 @@
>
> /// ObjCMethodScope - This scope corresponds to an Objective-C method
> body.
> /// It always has FnScope and DeclScope set as well.
> - ObjCMethodScope = 0x400
> + ObjCMethodScope = 0x400,
> +
> + /// SwitchScope - This is a scope that corresponds to a switch
> statement.
> + SwitchScope = 0x800
> };
> private:
> /// The parent scope for this scope. This is null for the
> translation-unit
> @@ -260,6 +263,14 @@
> return getFlags() & Scope::AtCatchScope;
> }
>
> + /// isSwitchScope - Return true if this scope is a switch scope.
> + bool isSwitchScope() const {
> + for (const Scope *S = this; S; S = S->getParent()) {
> + if (S->getFlags() & Scope::SwitchScope)
> + return true;
> + }
> + }
> +
>
> This is going to search all the way up the scope stack for a switch
> anywhere, which isn't necessarily the same thing as being in a switch
> statement because there could be inner classes/blocks/etc. For example,
> we'll incorrectly suggest the 'case' keyword for this example:
>
> void f(int x) {
> switch (x) {
> case 1: {
> struct Inner {
> void g() {
> 1: x = 17;
> }
> };
> break;
> }
> }
> }
>
> I see two solutions:
> 1) Prevent isSwitchScope() from walking through function
> declarations/blocks/etc. Or, only jump up one scope level (e.g., from the
> compound statement out to the switch) when checking for a switch scope,
> since case statements rarely show up anywhere else.
> 2) Add a Sema function isInSwitchStatement() and use that in the parser.
>
Went with solution 1 and changed isSwitchScope() so that it stops walking
through declaration/block/etc boundaries. Moved above code sample to test
case.
>
> @@ -251,8 +266,11 @@
> /// 'case' constant-expression ':' statement
> /// [GNU] 'case' constant-expression '...' constant-expression ':'
> statement
> ///
> -StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs) {
> - assert(Tok.is(tok::kw_case) && "Not a case stmt!");
> +StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs, bool
> MissingCase,
> + ExprResult Expr) {
> + if (!MissingCase) {
> + assert(Tok.is(tok::kw_case) && "Not a case stmt!");
> + }
>
> How about:
>
> assert((MissingCase || Tok.is(tok::kw_case)) && "Not a case stmt!");
>
>
> @@ -133,6 +136,18 @@
> ConsumeToken();
> return StmtError();
> }
> +
> + if (Tok.is(tok::colon) && getCurScope()->isSwitchScope() &&
> + Expr.get()->isIntegerConstantExpr(Actions.Context)) {
>
> There are two issues here. The first is that Expr::isIntegerConstantExpr()
> isn't safe for type- or value-dependent expressions, so we now crash on this
> ill-formed code:
>
> template<typename T>
> struct X {
> enum { E };
>
> void f(int x) {
> switch (x) {
> E: break;
> E+1: break;
> }
> }
> };
>
> The second issue is that the parser shouldn't probe the AST directly.
> Instead, please add a function into Sema that performs the semantic analysis
> and decides whether this expression was meant to be part of a case
> statement. That function should allow the correction for type-dependent
> expressions, value-dependent expressions with integral or enumeration type,
> and non-dependent, integral constant expressions.
>
Moved AST checks to Sema. Included checks for type and value dependent
expressions. Included above code into test case.
>
>
Finally, I had two thoughts for follow-on patches:
>
> 1) Given code like this:
>
> enum E { A };
> void f(int e) {
> switch (e) {
> A: break;
> }
> }
>
> Under -Wunused-label, we warn about 'A'. However, it would be very cool to
> give a warning like:
>
> warning: unused label 'A' also refers to an
> %select{integeral|enumeration}0 value within a switch statement
>
> note: did you mean to make this a case statement?
>
> (with a "case " Fix-It on the note).
>
>
> 2) It occurs to me that, if we're in a non-switch statement context and we
> we see a ':' after an expression, the ':' is probably a typo for ';'. It may
> be worth adding that recovery + Fix-It as well.
>
I think Clang already suggests a semi-colon when an out of place colon is
found. That was what it suggested before this patch.
>
> Thanks for working on this!
>
> - Doug
>
>
>
Index: test/Parser/switch-recovery.cpp
===================================================================
--- test/Parser/switch-recovery.cpp (revision 129825)
+++ test/Parser/switch-recovery.cpp (working copy)
@@ -31,4 +31,128 @@
break;
}
}
+
+ int test3(int i) {
+ switch (i) {
+ case 1: return 0;
+ 2: return 1; // expected-error {{expected 'case' keyword before expression}}
+ default: return 5;
+ }
+ }
};
+
+int test4(int i) {
+ switch (i)
+ 1: return -1; // expected-error {{expected 'case' keyword before expression}}
+ return 0;
+}
+
+int test5(int i) {
+ switch (i) {
+ case 1: case 2: case 3: return 1;
+ {
+ 4:5:6:7: return 2; // expected-error 4{{expected 'case' keyword before expression}}
+ }
+ default: return -1;
+ }
+}
+
+int test6(int i) {
+ switch (i) {
+ case 1:
+ case 4:
+ // This class provides extra single colon tokens. Make sure no
+ // errors are seen here.
+ class foo{
+ public:
+ protected:
+ private:
+ };
+ case 2:
+ 5: // expected-error {{expected 'case' keyword before expression}}
+ default: return 1;
+ }
+}
+
+int test7(int i) {
+ switch (i) {
+ case false ? 1 : 2:
+ true ? 1 : 2: // expected-error {{expected 'case' keyword before expression}}
+ case 10:
+ 14 ? 3 : 4;
+ default:
+ return 1;
+ }
+}
+
+enum foo { A, B, C};
+int test8( foo x ) {
+ switch (x) {
+ A: return 0; // no warning since A is a valid label.
+ default: return 1;
+ }
+}
+
+// Stress test to make sure Clang doesn't crash.
+void test9(int x) {
+ switch(x) {
+ case 1: return;
+ 2: case; // expected-error {{expected 'case' keyword before expression}} \
+ expected-error {{expected expression}}
+ 4:5:6: return; // expected-error 3{{expected 'case' keyword before expression}}
+ 7: :x; // expected-error {{expected 'case' keyword before expression}} \
+ expected-error {{expected expression}}
+ 8:: x; // expected-error {{expected ';' after expression}} \
+ expected-error {{no member named 'x' in the global namespace}} \
+ expected-warning {{expression result unused}}
+ 9:: :y; // expected-error {{expected ';' after expression}} \
+ expected-error {{expected unqualified-id}} \
+ expected-warning {{expression result unused}}
+ :; // expected-error {{expected expression}}
+ ::; // expected-error {{expected unqualified-id}}
+ }
+}
+
+void test10(int x) {
+ switch (x) {
+ case 1: {
+ struct Inner {
+ void g(int y) {
+ 2: y++; // expected-error {{expected ';' after expression}} \
+ // expected-warning {{expression result unused}}
+ }
+ };
+ break;
+ }
+ }
+}
+
+template<typename T>
+struct test11 {
+ enum { E };
+
+ void f(int x) {
+ switch (x) {
+ E: break; // FIXME: give a 'case' fix-it for unused labels that
+ // could also be an expression an a case label.
+ E+1: break; // expected-error {{expected 'case' keyword before expression}}
+ }
+ }
+};
+
+void test12(int x) {
+ switch (x) {
+ 0: // expected-error {{expected 'case' keyword before expression}}
+ while (x) {
+ 1: // expected-error {{expected 'case' keyword before expression}}
+ for (;x;) {
+ 2: // expected-error {{expected 'case' keyword before expression}}
+ if (x > 0) {
+ 3: // expected-error {{expected 'case' keyword before expression}}
+ --x;
+ }
+ }
+ }
+ }
+}
+
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td (revision 129825)
+++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
@@ -204,6 +204,9 @@
def err_unspecified_vla_size_with_static : Error<
"'static' may not be used with an unspecified variable length array size">;
+def err_expected_case_before_expression: Error<
+ "expected 'case' keyword before expression">;
+
// Declarations.
def err_typename_requires_specqual : Error<
"type name requires a specifier or qualifier">;
Index: include/clang/Sema/Scope.h
===================================================================
--- include/clang/Sema/Scope.h (revision 129825)
+++ include/clang/Sema/Scope.h (working copy)
@@ -75,7 +75,10 @@
/// ObjCMethodScope - This scope corresponds to an Objective-C method body.
/// It always has FnScope and DeclScope set as well.
- ObjCMethodScope = 0x400
+ ObjCMethodScope = 0x400,
+
+ /// SwitchScope - This is a scope that corresponds to a switch statement.
+ SwitchScope = 0x800
};
private:
/// The parent scope for this scope. This is null for the translation-unit
@@ -260,6 +263,20 @@
return getFlags() & Scope::AtCatchScope;
}
+ /// isSwitchScope - Return true if this scope is a switch scope.
+ bool isSwitchScope() const {
+ for (const Scope *S = this; S; S = S->getParent()) {
+ if (S->getFlags() & Scope::SwitchScope)
+ return true;
+ else if (S->getFlags() & (Scope::FnScope | Scope::ClassScope |
+ Scope::BlockScope | Scope::TemplateParamScope |
+ Scope::FunctionPrototypeScope |
+ Scope::AtCatchScope | Scope::ObjCMethodScope))
+ return false;
+ }
+ return false;
+ }
+
typedef UsingDirectivesTy::iterator udir_iterator;
typedef UsingDirectivesTy::const_iterator const_udir_iterator;
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 129825)
+++ include/clang/Sema/Sema.h (working copy)
@@ -2226,6 +2226,8 @@
// __null
ExprResult ActOnGNUNullExpr(SourceLocation TokenLoc);
+ bool CheckCaseExpression(Expr *expr);
+
//===------------------------- "Block" Extension ------------------------===//
/// ActOnBlockStart - This callback is invoked when a block literal is
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h (revision 129825)
+++ include/clang/Parse/Parser.h (working copy)
@@ -1238,7 +1238,9 @@
StmtResult ParseStatementOrDeclaration(StmtVector& Stmts,
bool OnlyStatement = false);
StmtResult ParseLabeledStatement(ParsedAttributes &Attr);
- StmtResult ParseCaseStatement(ParsedAttributes &Attr);
+ StmtResult ParseCaseStatement(ParsedAttributes &Attr,
+ bool MissingCase = false,
+ ExprResult Expr = ExprResult());
StmtResult ParseDefaultStatement(ParsedAttributes &Attr);
StmtResult ParseCompoundStatement(ParsedAttributes &Attr,
bool isStmtExpr = false);
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 129825)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -10734,3 +10734,8 @@
assert(!type->isPlaceholderType());
return Owned(E);
}
+
+bool Sema::CheckCaseExpression(Expr *expr) {
+ return expr->isTypeDependent() || expr->isValueDependent() ||
+ expr->isIntegerConstantExpr(Context);
+}
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 129825)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -121,6 +121,9 @@
return StmtError();
}
+ // If a case keyword is missing, this is where it should be inserted.
+ Token OldToken = Tok;
+
// FIXME: Use the attributes
// expression[opt] ';'
ExprResult Expr(ParseExpression());
@@ -133,6 +136,18 @@
ConsumeToken();
return StmtError();
}
+
+ if (Tok.is(tok::colon) && getCurScope()->isSwitchScope() &&
+ Actions.CheckCaseExpression(Expr.get())) {
+ // If a constant expression is followed by a colon inside a switch block,
+ // suggest a missing case keywork.
+ Diag(OldToken, diag::err_expected_case_before_expression)
+ << FixItHint::CreateInsertion(OldToken.getLocation(), "case ");
+
+ // Recover parsing as a case statement.
+ return ParseCaseStatement(attrs, /*MissingCase=*/true, Expr);
+ }
+
// Otherwise, eat the semicolon.
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
return Actions.ActOnExprStmt(Actions.MakeFullExpr(Expr.get()));
@@ -251,8 +266,9 @@
/// 'case' constant-expression ':' statement
/// [GNU] 'case' constant-expression '...' constant-expression ':' statement
///
-StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs) {
- assert(Tok.is(tok::kw_case) && "Not a case stmt!");
+StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs, bool MissingCase,
+ ExprResult Expr) {
+ assert(MissingCase || Tok.is(tok::kw_case) && "Not a case stmt!");
// FIXME: Use attributes?
// It is very very common for code to contain many case statements recursively
@@ -280,7 +296,8 @@
// While we have case statements, eat and stack them.
do {
- SourceLocation CaseLoc = ConsumeToken(); // eat the 'case'.
+ SourceLocation CaseLoc = MissingCase ? Expr.get()->getExprLoc() :
+ ConsumeToken(); // eat the 'case'.
if (Tok.is(tok::code_completion)) {
Actions.CodeCompleteCase(getCurScope());
@@ -292,7 +309,8 @@
/// expression.
ColonProtectionRAIIObject ColonProtection(*this);
- ExprResult LHS(ParseConstantExpression());
+ ExprResult LHS(MissingCase ? Expr : ParseConstantExpression());
+ MissingCase = false;
if (LHS.isInvalid()) {
SkipUntil(tok::colon);
return StmtError();
@@ -775,7 +793,7 @@
// while, for, and switch statements are local to the if, while, for, or
// switch statement (including the controlled statement).
//
- unsigned ScopeFlags = Scope::BreakScope;
+ unsigned ScopeFlags = Scope::BreakScope | Scope::SwitchScope;
if (C99orCXX)
ScopeFlags |= Scope::DeclScope | Scope::ControlScope;
ParseScope SwitchScope(this, ScopeFlags);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
