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

[Douglas Gregor]

On Apr 20, 2011, at 6:21 PM, Richard Trieu wrote:

> Another round of changes.  I believe I addressed all the issues raised.

Very nice! One last small request

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);
+}

When the expression is value-dependent or non-dependent, please check whether it has integral or enumeration type. We don't want to suggest "case" for, say, value-dependent expressions of pointer type.

You can go ahead and commit with that tweak.

	- Doug

> On Wed, Apr 20, 2011 at 7:35 AM, Douglas Gregor <dgregor at apple.com> 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.

Looks good!

> 
> @@ -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.

Okay!

	- Doug

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