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.
@@ -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.
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.
Thanks for working on this!
- Doug
- Doug
> On Fri, Apr 15, 2011 at 2:03 PM, Douglas Gregor <[email protected]> wrote:
>
> On Apr 15, 2011, at 11:58 AM, Richard Trieu wrote:
>
> > When a valid expression is followed by a colon inside a switch scope,
> > suggest a fix-it hint to insert a case before the expression.
> >
> > int f1(int i) {
> > switch (i) {
> > 0: return 1;
> > default: return 0;
> > }
> > }
> >
> > case.cc:3:4: error: expected 'case' keyword before expression
> > 0: return 1;
> > ^
> > case
>
> Cool idea, but this...
>
> + // If a case statement is missing, then back-track to this point and
> + // insert case keyword.
> + Token OldToken = Tok;
> + TentativeParsingAction TPA(*this);
>
> is a huge performance problem, since tentative parsing is expensive and
> should be avoided except after an error occurs or when required by the
> language.
>
> Is there a way to make this diagnostic kick in only when an error is
> imminent, e.g., because we've seen <expression> ':' somewhere within a switch
> statement?
>
> - Doug
>
> <missing-case-keyword2.patch>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
