r194869 - PR8455: Handle an attribute between a goto label and a variable declaration per
Richard Smith
richard at metafoo.co.uk
Fri Nov 15 17:32:28 PST 2013
On Fri, Nov 15, 2013 at 4:07 PM, Alp Toker <alp at nuanti.com> 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 <alp at nuanti.com <mailto:
>>> alp at nuanti.com>> 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
>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>> -- http://www.nuanti.com
>>> the browser experts
>>>
>>>
>>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131115/b8c84a32/attachment.html>
More information about the cfe-commits
mailing list