<div dir="ltr">Thank you for review!<div class="gmail_extra"><br><br><div class="gmail_quote">2014/1/8 Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span><br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Serge Pavlov <<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>> writes:<br>
> Due to statement expressions supported as GCC extension, it is possible<br>
> to put 'break' or 'continue' into a loop/switch statement but outside<br>
> its body, for example:<br>
><br>
>     for ( ; ({ if (first) { first = 0; continue; } 0; }); )<br>
><br>
> Such usage must be diagnosed as an error, GCC rejects it. With this<br>
> change clang interprets such constructs as GCC in C++ mode: 'break'<br>
> and 'continue' in the 3rd expression refer to the loop itself, in<br>
> the 1st and 2nd expressions - to outer loop.<br>
><br>
> This revision is reincarnation of <a href="http://llvm-reviews.chandlerc.com/D2018" target="_blank">http://llvm-reviews.chandlerc.com/D2018</a>, which was accidentally closed.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D2518" target="_blank">http://llvm-reviews.chandlerc.com/D2518</a><br>
><br>
> Files:<br>
>   include/clang/Sema/Scope.h<br>
>   lib/Parse/ParseStmt.cpp<br>
>   lib/Sema/Scope.cpp<br>
>   test/CodeGen/PR8880.c<br>
>   test/Parser/bad-control.c<br>
>   test/Sema/statements.c<br>
><br>
</div>> Index: include/clang/Sema/Scope.h<br>
> ===================================================================<br>
> --- include/clang/Sema/Scope.h<br>
> +++ include/clang/Sema/Scope.h<br>
> @@ -354,6 +354,16 @@<br>
>    /// Init - This is used by the parser to implement scope caching.<br>
>    ///<br>
>    void Init(Scope *parent, unsigned flags);<br>
> +<br>
> +  /// \brief Sets up the specified scope flags and adjusts the scope state<br>
> +  /// variables accordingly.<br>
> +  ///<br>
> +  void AddFlags(unsigned Flags);<br>
> +<br>
> +  /// \brief Clears the specified scope flags and adjusts the scope state<br>
> +  /// variables accordingly.<br>
> +  ///<br>
> +  void RemoveFlags(unsigned Flags);<br>
>  };<br>
><br>
>  }  // end namespace clang<br>
> Index: lib/Parse/ParseStmt.cpp<br>
> ===================================================================<br>
> --- lib/Parse/ParseStmt.cpp<br>
> +++ lib/Parse/ParseStmt.cpp<br>
> @@ -1170,7 +1170,7 @@<br>
>    // while, for, and switch statements are local to the if, while, for, or<br>
>    // switch statement (including the controlled statement).<br>
>    //<br>
> -  unsigned ScopeFlags = Scope::BreakScope | Scope::SwitchScope;<br>
> +  unsigned ScopeFlags = Scope::SwitchScope;<br>
>    if (C99orCXX)<br>
>      ScopeFlags |= Scope::DeclScope | Scope::ControlScope;<br>
>    ParseScope SwitchScope(this, ScopeFlags);<br>
> @@ -1208,6 +1208,7 @@<br>
>    // See comments in ParseIfStatement for why we create a scope for the<br>
>    // condition and a new scope for substatement in C++.<br>
>    //<br>
> +  getCurScope()->AddFlags(Scope::BreakScope);<br>
>    ParseScope InnerScope(this, Scope::DeclScope,<br>
>                          C99orCXX && Tok.isNot(tok::l_brace));<br>
><br>
> @@ -1259,12 +1260,9 @@<br>
>    // while, for, and switch statements are local to the if, while, for, or<br>
>    // switch statement (including the controlled statement).<br>
>    //<br>
> -  unsigned ScopeFlags;<br>
> +  unsigned ScopeFlags = 0;<br>
>    if (C99orCXX)<br>
> -    ScopeFlags = Scope::BreakScope | Scope::ContinueScope |<br>
> -                 Scope::DeclScope  | Scope::ControlScope;<br>
> -  else<br>
> -    ScopeFlags = Scope::BreakScope | Scope::ContinueScope;<br>
> +    ScopeFlags = Scope::DeclScope | Scope::ControlScope;<br>
>    ParseScope WhileScope(this, ScopeFlags);<br>
><br>
>    // Parse the condition.<br>
> @@ -1286,6 +1284,7 @@<br>
>    // See comments in ParseIfStatement for why we create a scope for the<br>
>    // condition and a new scope for substatement in C++.<br>
>    //<br>
> +  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);<br>
>    ParseScope InnerScope(this, Scope::DeclScope,<br>
>                          C99orCXX && Tok.isNot(tok::l_brace));<br>
><br>
> @@ -1337,6 +1336,7 @@<br>
><br>
>    // Pop the body scope if needed.<br>
>    InnerScope.Exit();<br>
> +  getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);<br>
><br>
>    if (Tok.isNot(tok::kw_while)) {<br>
>      if (!Body.isInvalid()) {<br>
> @@ -1419,12 +1419,9 @@<br>
>    // Names declared in the for-init-statement are in the same declarative-region<br>
>    // as those declared in the condition.<br>
>    //<br>
> -  unsigned ScopeFlags;<br>
> +  unsigned ScopeFlags = 0;<br>
>    if (C99orCXXorObjC)<br>
> -    ScopeFlags = Scope::BreakScope | Scope::ContinueScope |<br>
> -                 Scope::DeclScope  | Scope::ControlScope;<br>
> -  else<br>
> -    ScopeFlags = Scope::BreakScope | Scope::ContinueScope;<br>
> +    ScopeFlags = Scope::DeclScope | Scope::ControlScope;<br>
><br>
>    ParseScope ForScope(this, ScopeFlags);<br>
><br>
> @@ -1573,12 +1570,15 @@<br>
>      }<br>
><br>
>      // Parse the third part of the for specifier.<br>
> +    getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);<br>
>      if (Tok.isNot(tok::r_paren)) {   // for (...;...;)<br>
>        ExprResult Third = ParseExpression();<br>
>        // FIXME: The C++11 standard doesn't actually say that this is a<br>
>        // discarded-value expression, but it clearly should be.<br>
>        ThirdPart = Actions.MakeFullDiscardedValueExpr(Third.take());<br>
>      }<br>
> +  } else {<br>
> +    getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);<br>
>    }<br>
>    // Match the ')'.<br>
>    T.consumeClose();<br>
> @@ -1617,6 +1617,7 @@<br>
>    // See comments in ParseIfStatement for why we create a scope for<br>
>    // for-init-statement/condition and a new scope for substatement in C++.<br>
>    //<br>
> +  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);<br>
<br>
Aren't these flags already set, since you set them in both pahts of the<br>
if/else above?<br></blockquote><div><br></div><div>Yes, you are right. Removed. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>    ParseScope InnerScope(this, Scope::DeclScope,<br>
>                          C99orCXXorObjC && Tok.isNot(tok::l_brace));<br>
><br>
> Index: lib/Sema/Scope.cpp<br>
> ===================================================================<br>
> --- lib/Sema/Scope.cpp<br>
> +++ lib/Sema/Scope.cpp<br>
> @@ -69,3 +69,34 @@<br>
>    }<br>
>    return false;<br>
>  }<br>
> +<br>
> +void Scope::AddFlags(unsigned FlagsToSet) {<br>
> +  assert((FlagsToSet & ~(BreakScope | ContinueScope)) == 0 ||<br>
> +         "Unsupported scope flags");<br>
> +  assert((Flags & ControlScope) != 0 || "Must be control scope");<br>
<br>
I think you mean && here. None of these asserts will ever fire.<br></blockquote><div><br></div><div>Ops, you are absolutely right. And check for ControlScope is wrong, do loops haven't it.</div><div><br></div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  if (FlagsToSet & BreakScope) {<br>
> +    assert((Flags & BreakScope) == 0 || "Already set");<br>
> +    BreakParent = this;<br>
> +  }<br>
> +  if (FlagsToSet & ContinueScope) {<br>
> +    assert((Flags & ContinueScope) == 0 || "Already set");<br>
> +    ContinueParent = this;<br>
> +  }<br>
> +  Flags |= FlagsToSet;<br>
> +}<br>
> +<br>
> +void Scope::RemoveFlags(unsigned FlagsToClear) {<br>
> +  assert((FlagsToClear & ~(BreakScope | ContinueScope)) == 0 ||<br>
> +         "Unsupported scope flags");<br>
> +  if (FlagsToClear & BreakScope) {<br>
> +    assert((Flags & ControlScope) != 0 || "Must be control scope");<br>
> +    assert((Flags & BreakScope) != 0 || "Already cleared");<br>
> +    BreakParent = AnyParent->BreakParent;<br>
> + }<br>
> +  if (FlagsToClear & ContinueScope) {<br>
> +    assert((Flags & ControlScope) != 0 || "Must be control scope");<br>
> +    assert((Flags & ContinueScope) != 0 || "Already cleared");<br>
> +    ContinueParent = AnyParent->ContinueParent;<br>
> +  }<br>
> +  Flags &= ~FlagsToClear;<br>
> +}<br>
> Index: test/CodeGen/PR8880.c<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGen/PR8880.c<br>
> @@ -0,0 +1,93 @@<br>
> +// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s<br>
> +<br>
> +void pr8880_cg_1(int *iptr) {<br>
> +// CHECK-LABEL: define void @pr8880_cg_1(<br>
> +// CHECK-NEXT: entry:<br>
> +  int i, j;<br>
> +  for (i = 2; i != 10 ; i++ )<br>
> +// CHECK: for.cond{{[0-9]*}}:<br>
> +// CHECK: for.body{{[0-9]*}}:<br>
> +    for (j = 3 ; j < 22; (void)({ ++j; break; j;})) {<br>
> +// CHECK: for.cond{{[0-9]*}}:<br>
> +// CHECK: for.body{{[0-9]*}}:<br>
> +      *iptr = 7;<br>
> +// CHECK: for.inc{{[0-9]*}}:<br>
> +<br>
> +// break in 3rd expression of inner loop causes branch to end of inner loop<br>
> +<br>
> +// CHECK: br label %for.end[[INNER_END:[0-9]*]]<br>
> +// CHECK: for.end[[INNER_END]]:<br>
> +    }<br>
> +// CHECK: for.inc{{[0-9]*}}:<br>
> +// CHECK: for.end{{[0-9]*}}:<br>
> +// CHECK-NEXT: ret void<br>
> +}<br>
<br>
The labels you're keying on here won't have names in non-asserts builds,<br>
so these tests will fail.<br></blockquote><div><br></div><div>Important note, special thanks! I used different labels in the updated patch.</div><div> </div></div>-- <br>Thanks,<br>--Serge<br>
</div></div>