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

[Richard Trieu]

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

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.

>
> @@ -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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110420/bd05ab61/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: missing-case-keyword3.patch
Type: text/x-patch
Size: 10312 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110420/bd05ab61/attachment.bin>