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>