Thanks for making the changes and the commit! ~Aaron
On Fri, Nov 15, 2013 at 5:49 PM, Richard Smith <[email protected]> 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 <[email protected]> > 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 <[email protected]> >> 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 > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
