[PATCH] Attribute parsing following labels

[Aaron Ballman]

Thanks for making the changes and the commit!

~Aaron

On Fri, Nov 15, 2013 at 5:49 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> I made a few tweaks to the patch as I was reviewing, and I figured it'd be
> easier to just check that in than have you make the same tweaks and check
> in, so: committed with fixes for the below review comments as r194869.
>
> --- lib/Parse/ParseStmt.cpp (revision 193647)
> +++ lib/Parse/ParseStmt.cpp (working copy)
> [...]
> +  // attribute is immediately followed by a semi-colon to disambiguate it
> from
> +  // attributes on declarations.
>
> This isn't quite right: in C, the attributes *always* apply to the label
> (because a label cannot apply to a declaration). GCC only requires a
> semicolon after the label in C++ mode.
>
>
> +  StmtResult SubStmt;
> +  if (Tok.is(tok::kw___attribute)) {
> +    ParsedAttributesWithRange TempAttrs(AttrFactory);
> +    ParseGNUAttributes(TempAttrs);
>
> -  StmtResult SubStmt(ParseStatement());
> +    if (!TempAttrs.empty()) {
>
> With this check, "A: __attribute__(( )) int n;" won't parse the "int n;",
> and "A: __attribute__(( )) f();" won't trigger an error as it should. We
> should do what follows regardless, because we know we saw a kw___attribute.
>
>
> +      if (Tok.is(tok::semi))
> +        attrs.takeAllFrom(TempAttrs);
>
> We need to actually parse the substatement in this case.
>
>
> +      } else
> +        Diag(Tok, diag::err_expected_semi_after) << "__attribute__";
>
> Again, we need to parse the substatement in this case.
>
>
> On Wed, Oct 30, 2013 at 3:16 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Finally have some spare time to look into this again, thank you for
>> your patience.  :-)
>>
>> On Tue, Oct 15, 2013 at 12:46 AM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > +      SourceLocation Loc = Tok.getLocation();
>> >
>> > Unused variable.
>> >
>> > +      // If the SubStmt is invalid, we want the attributes to attach to
>> > the
>> > +      // label instead.
>> > +      if (!SubStmt.isUsable() || SubStmt.isInvalid())
>> > +        attrs.takeAllFrom(TempAttrs);
>> >
>> > I thought your experiments showed the attributes were ill-formed in this
>> > case.
>>
>> Since it's been a while, I went back to re-test my experiments, and I
>> think I made a mistake.
>>
>> void f() {
>>     A:
>>       __attribute__((unused)) int i; // attribute applies to variable
>>     B:
>>       __attribute__((unused)); // attribute applies to label
>>
>>     C:
>>       __attribute__((unused))
>>       #pragma weak unused_local_static
>>       ;
>>
>>     D:
>>       #pragma weak unused_local_static
>>       __attribute__((unused))
>>       ;
>> }
>>
>> *Both* of these cases generate errors in gcc. What I didn't realize is
>> that gcc stopped trying to report errors with D because C was
>> ill-formed.
>>
>> With D commented out, I get:
>>
>> prog.cpp: In function ‘void f()’:
>> prog.cpp:8:7: error: expected primary-expression before ‘__attribute__’
>>        __attribute__((unused))
>>        ^
>> prog.cpp:8:7: error: expected ‘;’ before ‘__attribute__’
>> prog.cpp:9:39: error: expected ‘}’ before end of line
>>        #pragma weak unused_local_static
>>                                        ^
>> prog.cpp:2:5: warning: label ‘A’ defined but not used [-Wunused-label]
>>      A:
>>      ^
>> prog.cpp:7:5: warning: label ‘C’ defined but not used [-Wunused-label]
>>      C:
>>      ^
>> prog.cpp: At global scope:
>> prog.cpp:9:39: error: expected declaration before end of line
>>        #pragma weak unused_local_static
>>
>> With C commented out, I get:
>>
>> prog.cpp: In function ‘void f()’:
>> prog.cpp:14:7: error: expected primary-expression before ‘__attribute__’
>>        __attribute__((unused))
>>        ^
>> prog.cpp:14:7: error: expected ‘;’ before ‘__attribute__’
>> prog.cpp:2:5: warning: label ‘A’ defined but not used [-Wunused-label]
>>      A:
>>      ^
>> prog.cpp:12:5: warning: label ‘D’ defined but not used [-Wunused-label]
>>      D:
>>      ^
>>
>> So I've created a new patch that I believe has the correct behavior.
>> If there's a semi-colon, the attributes are applied to the label.  If
>> there's a declaration, it is handled.  Any other form of statement
>> causes a specific error.
>>
>> Thanks!
>>
>> ~Aaron
>
>