This patch has been submitted as r129943<font class="Apple-style-span" face="monospace" size="3"><span class="Apple-style-span" style="white-space: pre;">.</span></font><br><meta http-equiv="content-type" content="text/html; charset=utf-8"><br>
<div class="gmail_quote">On Thu, Apr 21, 2011 at 7:30 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Apr 20, 2011, at 6:21 PM, Richard Trieu wrote:</div><br><blockquote type="cite">Another round of changes. I believe I addressed all the issues raised.<br>
</blockquote><div><br></div></div>Very nice! One last small request</div><div><br></div><div><div>Index: lib/Sema/SemaExpr.cpp</div><div>===================================================================</div><div>--- lib/Sema/SemaExpr.cpp<span style="white-space:pre-wrap"> </span>(revision 129825)</div>
<div>+++ lib/Sema/SemaExpr.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -10734,3 +10734,8 @@</div><div> assert(!type->isPlaceholderType());</div><div> return Owned(E);</div><div> }</div>
<div>+</div><div>+bool Sema::CheckCaseExpression(Expr *expr) {</div><div>+ return expr->isTypeDependent() || expr->isValueDependent() ||</div><div>+ expr->isIntegerConstantExpr(Context);</div><div>+}</div>
<div><br></div><div>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.</div>
<div><br></div></div><div>You can go ahead and commit with that tweak.</div><div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><div><div></div><div class="h5"><br><blockquote type="cite"><div class="gmail_quote">
On Wed, Apr 20, 2011 at 7:35 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Apr 19, 2011, at 5:19 PM, Richard Trieu wrote:</div>
<br><blockquote type="cite">I have reworked the program flow. Instead of tentative parsing, the already parsed expression is reused within the case statement parsing following colon detection.<br></blockquote><div><br></div>
</div><div>I like this much better! A few more comments:</div><div><br></div><div><div>Index: include/clang/Sema/Scope.h</div><div>===================================================================</div><div>--- include/clang/Sema/Scope.h<span style="white-space:pre-wrap"> </span>(revision 129825)</div>
<div>+++ include/clang/Sema/Scope.h<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -75,7 +75,10 @@</div><div> </div><div> /// ObjCMethodScope - This scope corresponds to an Objective-C method body.</div>
<div> /// It always has FnScope and DeclScope set as well.</div><div>- ObjCMethodScope = 0x400</div><div>+ ObjCMethodScope = 0x400,</div><div>+</div><div>+ /// SwitchScope - This is a scope that corresponds to a switch statement.</div>
<div>+ SwitchScope = 0x800</div><div> };</div><div> private:</div><div> /// The parent scope for this scope. This is null for the translation-unit</div><div>@@ -260,6 +263,14 @@</div><div> return getFlags() & Scope::AtCatchScope;</div>
<div> }</div><div> </div><div>+ /// isSwitchScope - Return true if this scope is a switch scope.</div><div>+ bool isSwitchScope() const {</div><div>+ for (const Scope *S = this; S; S = S->getParent()) {</div><div>
+ if (S->getFlags() & Scope::SwitchScope)</div><div>+ return true;</div><div>+ }</div><div>+ }</div><div>+</div><div><br></div><div>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:</div>
<div><br></div><div><div>void f(int x) {</div><div> switch (x) {</div><div> case 1: {</div><div> struct Inner {</div><div> void g() {</div><div> 1: x = 17;</div><div> }</div><div> };</div><div> break;</div>
<div> }</div><div> }</div><div>}</div><div><br></div></div><div>I see two solutions:</div><div> 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.</div>
<div> 2) Add a Sema function isInSwitchStatement() and use that in the parser.</div></div></div></div></blockquote><div> </div><div>Went with solution 1 and changed isSwitchScope() so that it stops walking through declaration/block/etc boundaries. Moved above code sample to test case.</div>
</div></blockquote><div><br></div></div></div>Looks good!</div><div><div></div><div class="h5"><div><br><blockquote type="cite"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div><div><br></div><div><div>@@ -251,8 +266,11 @@</div><div> /// 'case' constant-expression ':' statement</div>
<div> /// [GNU] 'case' constant-expression '...' constant-expression ':' statement</div><div> ///</div><div>-StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs) {</div><div>- assert(<a href="http://Tok.is/" target="_blank">Tok.is</a>(tok::kw_case) && "Not a case stmt!");</div>
<div>+StmtResult Parser::ParseCaseStatement(ParsedAttributes &attrs, bool MissingCase,</div><div>+ ExprResult Expr) {</div><div>+ if (!MissingCase) {</div><div>+ assert(<a href="http://Tok.is/" target="_blank">Tok.is</a>(tok::kw_case) && "Not a case stmt!");</div>
<div>+ }</div><div><br></div><div>How about:</div><div><br></div><div><span style="white-space:pre-wrap"> </span>assert((MissingCase || <a href="http://Tok.is/" target="_blank">Tok.is</a>(tok::kw_case)) && "Not a case stmt!");</div>
<div><br></div><div><br></div><div><div>@@ -133,6 +136,18 @@</div><div> ConsumeToken();</div><div> return StmtError();</div><div> }</div><div>+</div><div>+ if (<a href="http://Tok.is/" target="_blank">Tok.is</a>(tok::colon) && getCurScope()->isSwitchScope() &&</div>
<div>+ Expr.get()->isIntegerConstantExpr(Actions.Context)) {</div><div><br></div><div>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:</div>
<div><br></div><div><div>template<typename T></div><div>struct X {</div><div> enum { E };</div><div> </div><div> void f(int x) {</div><div> switch (x) {</div><div> E: break;</div><div> E+1: break;</div>
<div> }</div><div> }</div><div>};</div><div><br></div></div></div></div></div><div>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.</div>
</div></div></blockquote><div><br></div><div>Moved AST checks to Sema. Included checks for type and value dependent expressions. Included above code into test case. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div> </div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>
</div><div>Finally, I had two thoughts for follow-on patches:</div><div><br></div><div>1) Given code like this:</div><div><br></div><div>enum E { A };</div><div>void f(int e) {</div><div> switch (e) {</div><div> A: break;</div>
<div> }</div><div>}</div><div><br></div><div>Under -Wunused-label, we warn about 'A'. However, it would be very cool to give a warning like:</div><div><br></div><div> warning: unused label 'A' also refers to an %select{integeral|enumeration}0 value within a switch statement</div>
<div><br></div><div> note: did you mean to make this a case statement?</div><div><br></div><div>(with a "case " Fix-It on the note).</div><div><br></div><div><br></div><div>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.</div>
</div></div></blockquote><div>I think Clang already suggests a semi-colon when an out of place colon is found. That was what it suggested before this patch.</div></div></blockquote><br></div></div></div><div>Okay!</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><br></div></blockquote></div><br>