<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">2014/1/9 Richard Smith <span dir="ltr"><<a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a>></span><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"><div><div class="h5"><div>On Thu Dec 12 2013 at 9:20:37 PM, Serge Pavlov <<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>> wrote:</div><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"><div><div>2013/12/13 Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span><br><blockquote 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">
<br>
Have you checked that IR generation does the right thing in these cases? A CodeGen test for this would be useful.<br>
<div><br></div></blockquote></div></div></div><div dir="ltr"><div><div><div>Attached is a test that can be compiled and run to check particular binding in clang and gcc.</div></div></div></div><div dir="ltr"><div><div><div>
</div><blockquote 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>
<br>
================<br>
Comment at: test/Parser/bad-control.c:22-<u></u>30<br>
@@ +21,11 @@<br>
+<br>
+// GCC rejects this code<br>
+void pr8880_3(int first) {<br>
+ for ( ; ; (void)({ if (first) { first = 0; continue; } 0; })) {}<br>
+}<br>
+<br>
+// GCC rejects this code (see also tests/Analysis/dead-stores.c rdar8014335()<br>
+void pr8880_4(int first) {<br>
+ for ( ; ; (void)({ if (first) { first = 0; break; } 0; })) {}<br>
+}<br>
+<br>
----------------<br>
</div><div>Serge Pavlov wrote:<br>
> Richard Smith wrote:<br>
> > Does this mean we're misinterpreting pr8880_10 and pr8880_11 below?<br>
> No, this is a feature of GCC. It uses different interpretation depending on whether code is compiled as C or as C++. In C++ mode break and continue in the second or third expression of for statement refer to the inner loop, in C mode - to the outer. However in both modes GCC reject using break/continue if for statement is not inside another loop. Clang is more consistent, it considers that the third expression refers to the inner loop and the second - to outer, the interpretation is the same in C and C++ mode.<br>
</div>Interesting. I agree that it makes little sense for C and C++ to differ here, other than GCC compatibility.<br>
<br>
Why does break/continue in the second expression bind to the inner scope and the third expression bind to the outer? It seems strange that we would (deliberately) differ from both GCC's C mode and its C++ mode here.<br>
<br></blockquote></div></div></div><div dir="ltr"><div><div><div>Doug Gregor filed a bug against GCC (<a href="http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44715" target="_blank">http://gcc.gnu.org/bugzilla/<u></u>show_bug.cgi?id=44715</a>), but after 3 years GCC still keeps inconsistent behavior. Maybe he has more information on this subject.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I don't think you've answered my question.</div><div><br></div><div>GCC says that control in the second and third expression bind to the outer loop in C and to the inner loop in C++. You've picked another option that's not even self-consistent (binding to the outer loop for the second expression and to the inner loop for the third). Why are you doing that?</div>
</div></blockquote><div><br></div>There is nothing in GCC documentation that says which expression binds to which context. This correspondence was revealed experimentally. And there is PR44715 in GCC bug database that states GCC behavior is buggy.<div>
The dominating opinion in GCC community was that such constructs should be prohibited, but QT already uses 'break' in the 3rd expression of 'for' loop.</div><div>This patch does not chose particular binding to inner or outer scope, it only adds message generation if the corresponding scope is not a loop or switch. The binding is already implemented in clang. There must be reasons why the implementation is such, maybe Doug or Ted know them. Changing current implementation requires special care.</div>
<div class="gmail_extra"><br></div><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"><div><br>
</div><div>I think what we should do here is to always bind to the inner loop in the second and third expression, and we should additionally produce a -Wgcc-compat diagnostic if:</div><div>1) we're in C, and</div>
<div>2) we've got control flow in the second or third expression, and</div><div>3) there is an outer loop</div><div>(that is, in the case where both we and GCC would accept, and where we would do something different from what GCC does).</div>
</div></blockquote><div><br></div><div>As a variant, we could emit an error in all cases but break in the 3rd expression, just to allow QT code compile. These constructs can create problems for OpenMP or loop transformations if someone starts really using them.</div>
<div> </div><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"><div><div class="h5">
<div><br></div><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"><div></div><blockquote 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">
Since GCC rejects this if the loop is non-nested, it seems more reasonable to me for them to always bind to the surrounding scope. Is there a reason to not do that?<br></blockquote><div><br></div></div><div dir="ltr"><div>
<div><div>In C++ mode break/continue bind to inner scope. Surprisingly this construct is used in QT macro 'foreach'. The following code:</div>
<div><br></div><div><div> foreach(QString name, names)</div><div> qDebug() << name;</div></div><div><br></div><div>after preprocessing produces:</div><div><br></div><div><div>for (QForeachContainer<__typeof__(<u></u>names)> _container_(names); </div>
<div> !_container_.brk && _container_.i != _container_.e;</div><div> __extension__ ({ ++_container_.brk; ++_container_.i; }))</div><div>for (QString name = *_container_.i;; __extension__ ({--_container_.brk; break;}))</div>
<div> qDebug() << name;</div></div><div><br></div><div>So clang must bind break in the third expression to inner loop.</div><div><br></div><div>There is also a test in Analysis/dead-stores.c :</div><div><br></div>
<div><div>// The FOREACH macro in QT uses 'break' statements within statement expressions</div><div>// placed within the increment code of for loops.</div><div>void rdar8014335() {</div><div> for (int i = 0 ; i != 10 ; ({ break; })) {</div>
<div> for ( ; ; ({ ++i; break; })) ;</div><div> // Note that the next value stored to 'i' is never executed</div><div> // because the next statement to be executed is the 'break'</div><div> // in the increment code of the first loop.</div>
<div> i = i * 3; // expected-warning{{Value stored to 'i' is never read}} expected-warning{{The left operand to '*' is always 1}}</div><div> }</div><div>}</div></div><div><br></div><div>This code cannot be compiled by GCC (I tried 4.6.3 and ToT). The test was added by Ted Kremenek in r104375. Maybe he can recall if it was obtained from real application. If so, this is the second usage case clang should support.</div>
<div><br></div><div><br></div><blockquote 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">
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2018" target="_blank">http://llvm-reviews.chandlerc.<u></u>com/D2018</a><br>
</blockquote></div></div></div><div dir="ltr"><div><br><br clear="all"><div><br></div>-- <br>Thanks,<br>--Serge<br>
</div></div></blockquote></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Thanks,<br>--Serge<br>
</div></div>