<div dir="ltr"><div><div>@@ -1291,6 +1289,7 @@</div><div>   // See comments in ParseIfStatement for why we create a scope for the</div><div>   // condition and a new scope for substatement in C++.</div><div>   //</div><div>
+  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);</div><div>   ParseScope InnerScope(this, Scope::DeclScope,</div><div>                         C99orCXX && Tok.isNot(tok::l_brace));</div><div>
 </div><div>@@ -1342,6 +1341,7 @@</div><div> </div><div>   // Pop the body scope if needed.</div><div>   InnerScope.Exit();</div><div>+  getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);</div><div></div>
</div><div><br></div><div>These changes look redundant: you're adding and removing the flags to the outer scope for exactly the period where the outer scope is not active.<br></div><div><br></div><div><br></div><div>+  assert ((Flags & ControlScope) != 0 || "Must be control scope");</div>
<div><br></div><div>No space after 'assert'.</div><div><br></div><div><br></div><div><div>+void Scope::RemoveFlags(unsigned FlagsToClear) {</div><div>+  assert((FlagsToClear & ~(BreakScope | ContinueScope)) == 0 ||</div>
<div>+         "Unsupported scope flags");</div><div>+  if (FlagsToClear & BreakScope) {</div><div>+    assert((Flags & ControlScope) != 0 || "Must be control scope");</div><div>+    assert((Flags & BreakScope) != 0 || "Already cleared");</div>
<div>+    BreakParent = 0;</div><div>+ }</div><div>+  if (FlagsToClear & ContinueScope) {</div><div>+    assert ((Flags & ControlScope) != 0 || "Must be control scope");</div><div>+    assert((Flags & ContinueScope) != 0 || "Already cleared");</div>
<div>+    ContinueParent = 0;</div><div>+  }</div><div>+  Flags &= ~FlagsToClear;</div><div>+}</div></div><div><br></div><div>These are incorrect: the BreakParent and ContinueParent should be inherited from the parent here, rather than set to zero. However, with the first change above, RemoveFlags will be unused and can just be deleted.</div>
<div><br></div><div>You're missing test cases for 'break' and 'continue' in the condition of a do-while, which would have caught the above bug.<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Dec 9, 2013 at 4:06 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">If there is no objections, I'll commit this fix.</div></blockquote><div><br></div><div>See <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a> bullet 5: "Do not assume silent approval".</div>
</div></div></div>