Thank you for review!
2014/1/8 Justin Bogner <[email protected]> > Serge Pavlov <[email protected]> writes: > > Due to statement expressions supported as GCC extension, it is possible > > to put 'break' or 'continue' into a loop/switch statement but outside > > its body, for example: > > > > for ( ; ({ if (first) { first = 0; continue; } 0; }); ) > > > > Such usage must be diagnosed as an error, GCC rejects it. With this > > change clang interprets such constructs as GCC in C++ mode: 'break' > > and 'continue' in the 3rd expression refer to the loop itself, in > > the 1st and 2nd expressions - to outer loop. > > > > This revision is reincarnation of > http://llvm-reviews.chandlerc.com/D2018, which was accidentally closed. > > > > http://llvm-reviews.chandlerc.com/D2518 > > > > Files: > > include/clang/Sema/Scope.h > > lib/Parse/ParseStmt.cpp > > lib/Sema/Scope.cpp > > test/CodeGen/PR8880.c > > test/Parser/bad-control.c > > test/Sema/statements.c > > > > Index: include/clang/Sema/Scope.h > > =================================================================== > > --- include/clang/Sema/Scope.h > > +++ include/clang/Sema/Scope.h > > @@ -354,6 +354,16 @@ > > /// Init - This is used by the parser to implement scope caching. > > /// > > void Init(Scope *parent, unsigned flags); > > + > > + /// \brief Sets up the specified scope flags and adjusts the scope > state > > + /// variables accordingly. > > + /// > > + void AddFlags(unsigned Flags); > > + > > + /// \brief Clears the specified scope flags and adjusts the scope > state > > + /// variables accordingly. > > + /// > > + void RemoveFlags(unsigned Flags); > > }; > > > > } // end namespace clang > > Index: lib/Parse/ParseStmt.cpp > > =================================================================== > > --- lib/Parse/ParseStmt.cpp > > +++ lib/Parse/ParseStmt.cpp > > @@ -1170,7 +1170,7 @@ > > // while, for, and switch statements are local to the if, while, for, > or > > // switch statement (including the controlled statement). > > // > > - unsigned ScopeFlags = Scope::BreakScope | Scope::SwitchScope; > > + unsigned ScopeFlags = Scope::SwitchScope; > > if (C99orCXX) > > ScopeFlags |= Scope::DeclScope | Scope::ControlScope; > > ParseScope SwitchScope(this, ScopeFlags); > > @@ -1208,6 +1208,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); > > ParseScope InnerScope(this, Scope::DeclScope, > > C99orCXX && Tok.isNot(tok::l_brace)); > > > > @@ -1259,12 +1260,9 @@ > > // while, for, and switch statements are local to the if, while, for, > or > > // switch statement (including the controlled statement). > > // > > - unsigned ScopeFlags; > > + unsigned ScopeFlags = 0; > > if (C99orCXX) > > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope | > > - Scope::DeclScope | Scope::ControlScope; > > - else > > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope; > > + ScopeFlags = Scope::DeclScope | Scope::ControlScope; > > ParseScope WhileScope(this, ScopeFlags); > > > > // Parse the condition. > > @@ -1286,6 +1284,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)); > > > > @@ -1337,6 +1336,7 @@ > > > > // Pop the body scope if needed. > > InnerScope.Exit(); > > + getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope); > > > > if (Tok.isNot(tok::kw_while)) { > > if (!Body.isInvalid()) { > > @@ -1419,12 +1419,9 @@ > > // Names declared in the for-init-statement are in the same > declarative-region > > // as those declared in the condition. > > // > > - unsigned ScopeFlags; > > + unsigned ScopeFlags = 0; > > if (C99orCXXorObjC) > > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope | > > - Scope::DeclScope | Scope::ControlScope; > > - else > > - ScopeFlags = Scope::BreakScope | Scope::ContinueScope; > > + ScopeFlags = Scope::DeclScope | Scope::ControlScope; > > > > ParseScope ForScope(this, ScopeFlags); > > > > @@ -1573,12 +1570,15 @@ > > } > > > > // Parse the third part of the for specifier. > > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope); > > if (Tok.isNot(tok::r_paren)) { // for (...;...;) > > ExprResult Third = ParseExpression(); > > // FIXME: The C++11 standard doesn't actually say that this is a > > // discarded-value expression, but it clearly should be. > > ThirdPart = Actions.MakeFullDiscardedValueExpr(Third.take()); > > } > > + } else { > > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope); > > } > > // Match the ')'. > > T.consumeClose(); > > @@ -1617,6 +1617,7 @@ > > // See comments in ParseIfStatement for why we create a scope for > > // for-init-statement/condition and a new scope for substatement in > C++. > > // > > + getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope); > > Aren't these flags already set, since you set them in both pahts of the > if/else above? > Yes, you are right. Removed. > > ParseScope InnerScope(this, Scope::DeclScope, > > C99orCXXorObjC && Tok.isNot(tok::l_brace)); > > > > Index: lib/Sema/Scope.cpp > > =================================================================== > > --- lib/Sema/Scope.cpp > > +++ lib/Sema/Scope.cpp > > @@ -69,3 +69,34 @@ > > } > > return false; > > } > > + > > +void Scope::AddFlags(unsigned FlagsToSet) { > > + assert((FlagsToSet & ~(BreakScope | ContinueScope)) == 0 || > > + "Unsupported scope flags"); > > + assert((Flags & ControlScope) != 0 || "Must be control scope"); > > I think you mean && here. None of these asserts will ever fire. > Ops, you are absolutely right. And check for ControlScope is wrong, do loops haven't it. > > + if (FlagsToSet & BreakScope) { > > + assert((Flags & BreakScope) == 0 || "Already set"); > > + BreakParent = this; > > + } > > + if (FlagsToSet & ContinueScope) { > > + assert((Flags & ContinueScope) == 0 || "Already set"); > > + ContinueParent = this; > > + } > > + Flags |= FlagsToSet; > > +} > > + > > +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 = AnyParent->BreakParent; > > + } > > + if (FlagsToClear & ContinueScope) { > > + assert((Flags & ControlScope) != 0 || "Must be control scope"); > > + assert((Flags & ContinueScope) != 0 || "Already cleared"); > > + ContinueParent = AnyParent->ContinueParent; > > + } > > + Flags &= ~FlagsToClear; > > +} > > Index: test/CodeGen/PR8880.c > > =================================================================== > > --- /dev/null > > +++ test/CodeGen/PR8880.c > > @@ -0,0 +1,93 @@ > > +// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s > > + > > +void pr8880_cg_1(int *iptr) { > > +// CHECK-LABEL: define void @pr8880_cg_1( > > +// CHECK-NEXT: entry: > > + int i, j; > > + for (i = 2; i != 10 ; i++ ) > > +// CHECK: for.cond{{[0-9]*}}: > > +// CHECK: for.body{{[0-9]*}}: > > + for (j = 3 ; j < 22; (void)({ ++j; break; j;})) { > > +// CHECK: for.cond{{[0-9]*}}: > > +// CHECK: for.body{{[0-9]*}}: > > + *iptr = 7; > > +// CHECK: for.inc{{[0-9]*}}: > > + > > +// break in 3rd expression of inner loop causes branch to end of inner > loop > > + > > +// CHECK: br label %for.end[[INNER_END:[0-9]*]] > > +// CHECK: for.end[[INNER_END]]: > > + } > > +// CHECK: for.inc{{[0-9]*}}: > > +// CHECK: for.end{{[0-9]*}}: > > +// CHECK-NEXT: ret void > > +} > > The labels you're keying on here won't have names in non-asserts builds, > so these tests will fail. > Important note, special thanks! I used different labels in the updated patch. -- Thanks, --Serge
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
