On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <[email protected]> wrote: > > On 16/11/2013 00:00, Alp Toker wrote: > >> >> On 15/11/2013 23:42, Richard Smith wrote: >> >>> On Fri, Nov 15, 2013 at 3:33 PM, Alp Toker <[email protected] <mailto: >>> [email protected]>> wrote: >>> >>> Hello Richard, >>> >>> For the record I had a patch up for review to fix this back from >>> 2010, not sure if it ever reached the list: >>> >>> http://llvm.org/bugs/show_bug.cgi?id=8455 >>> >>> Just to compare notes, my approach was to use a >>> TentativeParsingAction to handle some of the corner cases. >>> >>> >>> Aaron's original patch went in that direction, but got reworked to avoid >>> the cost of a tentative parse. >>> >> >> But this *is* a disambig problem. The tentative parse can yield a more >> meaningful diag when encountering Microsoft-style attributes, such as those >> appearing in some MSVC headers. This outweighs the theoretical cost of a >> tentative parse IMO. >> > > In fact, this applies not just to Microsoft-style attributes but also > Objective-C messages using square brackets. > > Tentative parse was the only solution to handle this correctly in > Objective-C++ using GNU labeled statements with C++0x attributes when the > patch was written three years ago, and although the standards weren't final > yet I don't think that's changed. > > Have you actually tested corner cases like this?
The patch only applies to GNU attributes; they're the only ones we parse at this location. If there's a problem with __declspec attributes here, it's a different problem, since we don't look for them here. There's no problem with C++11 attributes or square brackets, since they don't go in this location when applied to a label. Your patch also only applied to GNU attributes. Judging that PR8455 was closed shortly after the commit, I'm guessing you >> or Aaron knew my patch existed beforehand. >> >> Please run it by me next time if a fix is already out there and you think >> there's a better approach so we can discuss it. >> > I only saw your patch after the fix was already committed. Our usual workflow does not involve posting patches to bugs; such patches will usually be ignored, since bug updates don't generate mail to llvmbugs. That's arguably a problem with the way bugzilla is set up... In any case, your approach had been discussed and we chose to go a different way, so even if we'd seen your patch it probably wouldn't have made much difference (except that I might have cc'd you on the review -- but I don't think it's reasonable of you to demand that treatment, since you can read this list as well as anyone else can). > Alp. >> >> Alp. >>> >>> >>> >>> >>> On 15/11/2013 22:45, Richard Smith wrote: >>> >>> Author: rsmith >>> Date: Fri Nov 15 16:45:29 2013 >>> New Revision: 194869 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=194869&view=rev >>> Log: >>> PR8455: Handle an attribute between a goto label and a >>> variable declaration per >>> the GNU documentation: the attribute only appertains to the >>> label if it is >>> followed by a semicolon. Based on a patch by Aaron Ballman! >>> >>> Modified: >>> cfe/trunk/lib/Parse/ParseStmt.cpp >>> cfe/trunk/test/Misc/ast-dump-attr.cpp >>> cfe/trunk/test/Sema/warn-unused-label.c >>> cfe/trunk/test/SemaCXX/warn-unused-variables.cpp >>> >>> Modified: cfe/trunk/lib/Parse/ParseStmt.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ >>> ParseStmt.cpp?rev=194869&r1=194868&r2=194869&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/lib/Parse/ParseStmt.cpp (original) >>> +++ cfe/trunk/lib/Parse/ParseStmt.cpp Fri Nov 15 16:45:29 2013 >>> @@ -516,11 +516,40 @@ StmtResult Parser::ParseLabeledStatement >>> // identifier ':' statement >>> SourceLocation ColonLoc = ConsumeToken(); >>> - // Read label attributes, if present. attrs will contain >>> both C++11 and GNU >>> - // attributes (if present) after this point. >>> - MaybeParseGNUAttributes(attrs); >>> + // Read label attributes, if present. >>> + StmtResult SubStmt; >>> + if (Tok.is(tok::kw___attribute)) { >>> + ParsedAttributesWithRange TempAttrs(AttrFactory); >>> + ParseGNUAttributes(TempAttrs); >>> + >>> + // In C++, GNU attributes only apply to the label if they >>> are followed by a >>> + // semicolon, to disambiguate label attributes from >>> attributes on a labeled >>> + // declaration. >>> + // >>> + // This doesn't quite match what GCC does; if the >>> attribute list is empty >>> + // and followed by a semicolon, GCC will reject (it >>> appears to parse the >>> + // attributes as part of a statement in that case). That >>> looks like a bug. >>> + if (!getLangOpts().CPlusPlus || Tok.is(tok::semi)) >>> + attrs.takeAllFrom(TempAttrs); >>> + else if (isDeclarationStatement()) { >>> + StmtVector Stmts; >>> + // FIXME: We should do this whether or not we have a >>> declaration >>> + // statement, but that doesn't work correctly (because >>> ProhibitAttributes >>> + // can't handle GNU attributes), so only call it in the >>> one case where >>> + // GNU attributes are allowed. >>> + SubStmt = ParseStatementOrDeclarationAfterAttributes( >>> + Stmts, /*OnlyStmts*/ true, 0, TempAttrs); >>> + if (!TempAttrs.empty() && !SubStmt.isInvalid()) >>> + SubStmt = Actions.ProcessStmtAttributes( >>> + SubStmt.get(), TempAttrs.getList(), >>> TempAttrs.Range); >>> + } else { >>> + Diag(Tok, diag::err_expected_semi_after) << >>> "__attribute__"; >>> + } >>> + } >>> - StmtResult SubStmt(ParseStatement()); >>> + // If we've not parsed a statement yet, parse one now. >>> + if (!SubStmt.isInvalid() && !SubStmt.isUsable()) >>> + SubStmt = ParseStatement(); >>> // Broken substmt shouldn't prevent the label from being >>> added to the AST. >>> if (SubStmt.isInvalid()) >>> @@ -557,7 +586,7 @@ StmtResult Parser::ParseCaseStatement(bo >>> // out of stack space in our recursive descent parser. As >>> a special case, >>> // flatten this recursion into an iterative loop. This is >>> complex and gross, >>> // but all the grossness is constrained to >>> ParseCaseStatement (and some >>> - // wierdness in the actions), so this is just local >>> grossness :). >>> + // weirdness in the actions), so this is just local >>> grossness :). >>> // TopLevelCase - This is the highest level we have >>> parsed. 'case 1' in the >>> // example above. >>> >>> Modified: cfe/trunk/test/Misc/ast-dump-attr.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ >>> ast-dump-attr.cpp?rev=194869&r1=194868&r2=194869&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/Misc/ast-dump-attr.cpp (original) >>> +++ cfe/trunk/test/Misc/ast-dump-attr.cpp Fri Nov 15 16:45:29 >>> 2013 >>> @@ -95,3 +95,19 @@ void *TestVariadicUnsigned1(int) __attri >>> void *TestVariadicUnsigned2(int, int) >>> __attribute__((alloc_size(1,2))); >>> // CHECK: FunctionDecl{{.*}}TestVariadicUnsigned2 >>> // CHECK: AllocSizeAttr{{.*}} 0 1 >>> + >>> +void TestLabel() { >>> +L: __attribute__((unused)) int i; >>> +// CHECK: LabelStmt{{.*}}'L' >>> +// CHECK: VarDecl{{.*}}i 'int' >>> +// CHECK-NEXT: UnusedAttr{{.*}} >>> + >>> +M: __attribute(()) int j; >>> +// CHECK: LabelStmt {{.*}} 'M' >>> +// CHECK-NEXT: DeclStmt >>> +// CHECK-NEXT: VarDecl {{.*}} j 'int' >>> + >>> +N: __attribute(()) ; >>> +// CHECK: LabelStmt {{.*}} 'N' >>> +// CHECK-NEXT: NullStmt >>> +} >>> >>> Modified: cfe/trunk/test/Sema/warn-unused-label.c >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/ >>> warn-unused-label.c?rev=194869&r1=194868&r2=194869&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/Sema/warn-unused-label.c (original) >>> +++ cfe/trunk/test/Sema/warn-unused-label.c Fri Nov 15 >>> 16:45:29 2013 >>> @@ -9,3 +9,7 @@ void f() { >>> goto d; >>> return; >>> } >>> + >>> +void PR8455() { >>> + L: __attribute__((unused)) return; // ok, no semicolon >>> required >>> +} >>> >>> Modified: cfe/trunk/test/SemaCXX/warn-unused-variables.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ >>> SemaCXX/warn-unused-variables.cpp?rev=194869&r1=194868&r2= >>> 194869&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/SemaCXX/warn-unused-variables.cpp (original) >>> +++ cfe/trunk/test/SemaCXX/warn-unused-variables.cpp Fri Nov >>> 15 16:45:29 2013 >>> @@ -1,4 +1,4 @@ >>> -// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -verify %s >>> +// RUN: %clang_cc1 -fsyntax-only -Wunused-variable >>> -Wunused-label -verify %s >>> template<typename T> void f() { >>> T t; >>> t = 17; >>> @@ -128,3 +128,26 @@ namespace ctor_with_cleanups { >>> } >>> #include "Inputs/warn-unused-variables.h" >>> + >>> +namespace PR8455 { >>> + void f() { >>> + A: // expected-warning {{unused label 'A'}} >>> + __attribute__((unused)) int i; // attribute applies to >>> variable >>> + B: // attribute applies to label >>> + __attribute__((unused)); int j; // expected-warning >>> {{unused variable 'j'}} >>> + } >>> + >>> + void g() { >>> + C: // unused label 'C' will not appear here because an >>> error occurs >>> + __attribute__((unused)) >>> + #pragma weak unused_local_static // expected-error >>> {{expected ';' after __attribute__}} >>> + ; >>> + } >>> + >>> + void h() { >>> + D: // expected-warning {{unused label 'D'}} >>> + #pragma weak unused_local_static >>> + __attribute__((unused)) // expected-warning >>> {{declaration does not declare anything}} >>> + ; >>> + } >>> +} >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] <mailto:[email protected]> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >>> -- http://www.nuanti.com >>> the browser experts >>> >>> >>> >> > -- > http://www.nuanti.com > the browser experts > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
