On Wed, Jan 8, 2014 at 10:36 PM, Serge Pavlov <[email protected]> wrote:
> > 2014/1/9 Richard Smith <[email protected]> > >> On Thu Dec 12 2013 at 9:20:37 PM, Serge Pavlov <[email protected]> >> wrote: >> >>> 2013/12/13 Richard Smith <[email protected]> >>> >>> >>> Have you checked that IR generation does the right thing in these >>> cases? A CodeGen test for this would be useful. >>> >>> Attached is a test that can be compiled and run to check particular >>> binding in clang and gcc. >>> >>> >>> >>> ================ >>> Comment at: test/Parser/bad-control.c:22-30 >>> @@ +21,11 @@ >>> + >>> +// GCC rejects this code >>> +void pr8880_3(int first) { >>> + for ( ; ; (void)({ if (first) { first = 0; continue; } 0; })) {} >>> +} >>> + >>> +// GCC rejects this code (see also tests/Analysis/dead-stores.c >>> rdar8014335() >>> +void pr8880_4(int first) { >>> + for ( ; ; (void)({ if (first) { first = 0; break; } 0; })) {} >>> +} >>> + >>> ---------------- >>> Serge Pavlov wrote: >>> > Richard Smith wrote: >>> > > Does this mean we're misinterpreting pr8880_10 and pr8880_11 below? >>> > 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. >>> Interesting. I agree that it makes little sense for C and C++ to differ >>> here, other than GCC compatibility. >>> >>> 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. >>> >>> Doug Gregor filed a bug against GCC (http://gcc.gnu.org/bugzilla/ >>> show_bug.cgi?id=44715), but after 3 years GCC still keeps inconsistent >>> behavior. Maybe he has more information on this subject. >>> >> >> I don't think you've answered my question. >> >> 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? >> > > 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. > 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. > 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. > > >> 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: >> 1) we're in C, and >> 2) we've got control flow in the second or third expression, and >> 3) there is an outer loop >> (that is, in the case where both we and GCC would accept, and where we >> would do something different from what GCC does). >> > > 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. > I'd be OK with that (or perhaps with allowing both 'break' and 'continue' in the third expression) -- disallowing the cases where we currently have different behavior from GCC seems unlikely to break much existing code. I'd still want the warning if this happens in C code, though. > 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? >>> >>> >>> In C++ mode break/continue bind to inner scope. Surprisingly this >>> construct is used in QT macro 'foreach'. The following code: >>> >>> foreach(QString name, names) >>> qDebug() << name; >>> >>> after preprocessing produces: >>> >>> for (QForeachContainer<__typeof__(names)> _container_(names); >>> !_container_.brk && _container_.i != _container_.e; >>> __extension__ ({ ++_container_.brk; ++_container_.i; })) >>> for (QString name = *_container_.i;; __extension__ ({--_container_.brk; >>> break;})) >>> qDebug() << name; >>> >>> So clang must bind break in the third expression to inner loop. >>> >>> There is also a test in Analysis/dead-stores.c : >>> >>> // The FOREACH macro in QT uses 'break' statements within statement >>> expressions >>> // placed within the increment code of for loops. >>> void rdar8014335() { >>> for (int i = 0 ; i != 10 ; ({ break; })) { >>> for ( ; ; ({ ++i; break; })) ; >>> // Note that the next value stored to 'i' is never executed >>> // because the next statement to be executed is the 'break' >>> // in the increment code of the first loop. >>> i = i * 3; // expected-warning{{Value stored to 'i' is never read}} >>> expected-warning{{The left operand to '*' is always 1}} >>> } >>> } >>> >>> 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. >>> >>> >>> >>> >>> http://llvm-reviews.chandlerc.com/D2018 >>> >>> >>> >>> >>> -- >>> Thanks, >>> --Serge >>> >> > > > -- > Thanks, > --Serge >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
