On Wed, Dec 11, 2013 at 9:55 AM, Serge Pavlov <[email protected]> wrote:
> > 2013/12/10 Richard Smith <[email protected]> > >> @@ -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. > OK, I see. In C89, declarations from within the controlled statement *are* visible in the while condition, but 'break' and 'continue' affect the outer loop, in hideous contortions like this: extern void *p; void f() { while(1) { do (struct S { int n; }*)0; while ( ((struct S*)p)->n && // use struct definition from within loop ({ break; 0; }) // break out of containing 'while' loop ); } } ... so we do need to RemoveFlags. Yuck. + 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 <[email protected]> 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
