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?

Alp.



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.

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

Reply via email to