<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">2013/12/10 Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div>
The outer scope defines BreakParent and ContinueParent for the inner scope. Setting flags to the inner scope is not possible in some cases. If the loop body is represented by compound statement, inner scope is made by the latter and break/continue flags wouldn't be set.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></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></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div></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></div></div></blockquote>
<div>Sadly but you are absolutely right. Thank you for the catch. I tried making better test coverage.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><br><div class="gmail_quote"><div class="im">
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><div>See <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" target="_blank">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a> bullet 5: "Do not assume silent approval".</div>

</div></div></div>
</blockquote></div>Yes, sure. Please forgive me my impatience.<br><br clear="all"><div><br></div>-- <br>Thanks,<br>--Serge<br>
</div></div>