[cfe-commits] Patch - Add fix-it hint for missing case keyword within a switch scope

[Douglas Gregor]

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 <dgregor at apple.com> 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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110420/b5d5a68f/attachment.html>