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 <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
