[PATCH] Fix to PR8880 (clang dies processing a for loop).

[Serge Pavlov]

2013/12/10 Richard Smith <richard at metafoo.co.uk>

> @@ -1291,6 +1289,7 @@
>    // See comments in ParseIfStatement for why we create a scope for the
>    // condition and a new scope for substatement in C++.
>    //
> +  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
>    ParseScope InnerScope(this, Scope::DeclScope,
>                          C99orCXX && Tok.isNot(tok::l_brace));
>
> @@ -1342,6 +1341,7 @@
>
>    // Pop the body scope if needed.
>    InnerScope.Exit();
> +  getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);
>
> 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.
>
> 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.

>
> +  assert ((Flags & ControlScope) != 0 || "Must be control scope");
>
> No space after 'assert'.
>
> Fixed.

>
> +void Scope::RemoveFlags(unsigned FlagsToClear) {
> +  assert((FlagsToClear & ~(BreakScope | ContinueScope)) == 0 ||
> +         "Unsupported scope flags");
> +  if (FlagsToClear & BreakScope) {
> +    assert((Flags & ControlScope) != 0 || "Must be control scope");
> +    assert((Flags & BreakScope) != 0 || "Already cleared");
> +    BreakParent = 0;
> + }
> +  if (FlagsToClear & ContinueScope) {
> +    assert ((Flags & ControlScope) != 0 || "Must be control scope");
> +    assert((Flags & ContinueScope) != 0 || "Already cleared");
> +    ContinueParent = 0;
> +  }
> +  Flags &= ~FlagsToClear;
> +}
>
> 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.
>
> You're missing test cases for 'break' and 'continue' in the condition of a
> do-while, which would have caught the above bug.
>
> Sadly but you are absolutely right. Thank you for the catch. I tried
making better test coverage.

>
> On Mon, Dec 9, 2013 at 4:06 AM, Serge Pavlov <sepavloff at gmail.com> wrote:
>
>> If there is no objections, I'll commit this fix.
>>
>
> See http://llvm.org/docs/DeveloperPolicy.html#code-reviews bullet 5: "Do
> not assume silent approval".
>
Yes, sure. Please forgive me my impatience.


-- 
Thanks,
--Serge
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131212/ab46f4cc/attachment.html>