[PATCH] D63299: [Clang] Parse GNU fallthrough attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Parse/ParseStmt.cpp:102
   ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseGNUAttributes(Attrs);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > Maybe we check if Tok is kw__attribute and look ahead a few tokens to check if attribute name is fallthough in ParseStatementOrDeclarationAfterAttributes.
> > 
> > Now, we always fall into
> > 
> > if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
> >          (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
> >              ParsedStmtContext()) &&
> >         isDeclarationStatement()) {
> >       SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
> >       DeclGroupPtrTy Decl = ParseDeclaration(DeclaratorContext::BlockContext,
> >                                              DeclEnd, Attrs);
> >       return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
> >   }
> Then we go to "ParseSimpleDeclaration" -> "ParseDeclarationSpecifier". What is quite strange for me, we do not set "Attrs" in ParseSimpleDeclaration from DS.getAttributes()..
> Maybe we check if Tok is kw__attribute and look ahead a few tokens to check if attribute name is fallthough in ParseStatementOrDeclarationAfterAttributes.

Why not check whether the attribute is a statement attribute or a declaration attribute, rather than tying it to fallthrough specifically?

But the result then would be: if it's a statement attribute, we should be trying to parse it as a statement rather than declaration statement; if it's a declaration attribute, we should try to parse as a declaration. This seems very much like spooky action at a distance -- attributes should not dictate whether something is parsed as a decl or a stmt.

That said, this really is the right place for that code to go, I think. We've never had a GNU null attribute statement before, so the parser may be written in an inconvenient way to support that (IIRC, we have similar deficiencies with things like attributes appertaining to declaration specifiers -- we don't have attributes that need it yet, so support was never added for it).

This may require a bit more surgery to fix up, unfortunately.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63299/new/

https://reviews.llvm.org/D63299