[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D64838#1589770 <https://reviews.llvm.org/D64838#1589770>, @Nathan-Huckleberry wrote:

> The main problem that we have is that the `__attribute__` token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.
>
> This case demonstrates the problem:
>
>   void foo() {
>     __attribute__((address_space(0))) *x;
>   }
>
>
> If the attribute token is read beforehand this code just becomes `*x` and is parsed as a dereference of an undefined variable when it should actually be a declaration.
>
> Maybe the best solution is to pull the attributes parsing out to `ParseStatementOrDeclaration` then pass those attributes through to following functions. 
>  When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.
>
> Does that sound like a good solution?


Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- if I understand your approach properly, this approach leads to spooky action at a distance, where the name of the attribute dictates whether something is a statement or a declaration. I'm not comfortable with that. There's no reason we could not have an attribute that is both a statement attribute and a declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that attributes do not impact whether something is treated as a declaration or a statement. The parser should parse attributes first, keep them around for a while, and attach them to either the decl or the statement at a later point.



================
Comment at: clang/lib/Parse/ParseStmt.cpp:104
+
+  if (isNullStmtWithAttributes()) {
+    MaybeParseGNUAttributes(Attrs);
----------------
efriedma wrote:
> If we're going to generally support statement attributes, it should be possible to apply them to non-null statements.  (Even if there aren't any valid attributes right now, that's likely to change in the future.)  Or is that not possible to implement for some reason?
We already generally support statement attributes. See `ProcessStmtAttribute()` in SemaStmtAttr.cpp. They definitely need to apply to non-null statements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64838