<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Apr 20, 2011, at 6:21 PM, Richard Trieu wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Another round of changes. I believe I addressed all the issues raised.<br></blockquote><div><br></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 class="Apple-tab-span" style="white-space:pre"> </span>(revision 129825)</div><div>+++ lib/Sema/SemaExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </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 class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><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">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 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>Looks good!</div><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; position: static; z-index: auto; "><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>Okay!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><br></body></html>