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

[Richard Trieu]

This patch has been submitted as r129943.

On Thu, Apr 21, 2011 at 7:30 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> 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/e6d61583/attachment.html>