<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Apr 19, 2011, at 5:19 PM, Richard Trieu wrote:</div><br class="Apple-interchange-newline"><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>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 class="Apple-tab-span" style="white-space:pre"> </span>(revision 129825)</div><div>+++ include/clang/Sema/Scope.h<span class="Apple-tab-span" style="white-space:pre"> </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><br></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">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">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 class="Apple-tab-span" style="white-space:pre"> </span>assert((MissingCase || <a href="http://Tok.is">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">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><br></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><br></div><div>Thanks for working on this!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br></div><div><br></div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br></div><div><br></div><br><blockquote type="cite"><div class="gmail_quote">On Fri, Apr 15, 2011 at 2:03 PM, 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><div></div><div class="h5"><br>
On Apr 15, 2011, at 11:58 AM, Richard Trieu wrote:<br>
<br>
> When a valid expression is followed by a colon inside a switch scope, suggest a fix-it hint to insert a case before the expression.<br>
><br>
> int f1(int i) {<br>
> switch (i) {<br>
> 0: return 1;<br>
> default: return 0;<br>
> }<br>
> }<br>
><br>
> case.cc:3:4: error: expected 'case' keyword before expression<br>
> 0: return 1;<br>
> ^<br>
> case<br>
<br>
</div></div>Cool idea, but this...<br>
<br>
+ // If a case statement is missing, then back-track to this point and<br>
+ // insert case keyword.<br>
+ Token OldToken = Tok;<br>
+ TentativeParsingAction TPA(*this);<br>
<br>
is a huge performance problem, since tentative parsing is expensive and should be avoided except after an error occurs or when required by the language.<br>
<br>
Is there a way to make this diagnostic kick in only when an error is imminent, e.g., because we've seen <expression> ':' somewhere within a switch statement?<br>
<br>
- Doug<br>
</blockquote></div><br>
<span><missing-case-keyword2.patch></span></blockquote></div><br></body></html>