[PATCH] D93630: [Attr] Apply GNU-style attributes to expression statements

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D93630#2479260 <https://reviews.llvm.org/D93630#2479260>, @aaron.ballman wrote:

> In D93630#2478977 <https://reviews.llvm.org/D93630#2478977>, @vsavchenko wrote:
>
>> I guess I want to clarify one point here, after this patch the parser **will not assume** that statement always follows statement attributes.  We simply turn off the assumption that what follows is a declaration, parser will simply determine whether it is a statement or a declaration, it can do it on its own without attribute's "help".
>
> We used to say that if there's a GNU attribute (or we're parsing a declaration statement), what follows must be a declaration. Now we're using the attributes in the given attribute list to decide whether what follows is a declaration or a statement (if all of the attributes are statement attributes, we don't treat what follows as being a declaration unless we're in a declaration statement). Or am I misreading the code?

We do not decide whether it is declaration or statement.  I do get that it is a matter of perspective, but right now I prefer to read it like this:
**BEFORE**: if we've seen a GNU-style attribute parse whatever comes next as a declaration
**AFTER**: if we've seen a GNU-style attribute except for the case when all of the parsed attributes are known to be statement attributes, parse whatever comes next as a declaration

So, essentially, when we see statement attributes only, attribute/declaration heuristic is canceled and we parse exactly the way we would've parsed as if no attributes present.

> e.g.,
>
>   void foo(void) {
>     __attribute__((fallthrough)) i = 12;
>   }
>
> Before patch:
>
>   F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron Ballman\Desktop\test.c"
>   F:\Aaron Ballman\Desktop\test.c:2:32: warning: type specifier missing, defaults to 'int' [-Wimplicit-int]
>     __attribute__((fallthrough)) i = 12;
>                                  ^
>   F:\Aaron Ballman\Desktop\test.c:2:18: error: 'fallthrough' attribute cannot be applied to a declaration
>     __attribute__((fallthrough)) i = 12;
>                    ^             ~
>   1 warning and 1 error generated.
>
> After patch:
>
>   F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron Ballman\Desktop\test.c"
>   F:\Aaron Ballman\Desktop\test.c:2:32: error: use of undeclared identifier 'i'
>     __attribute__((fallthrough)) i = 12;
>                                  ^
>   1 error generated.

In both situations, the code contains errors.  I don't think that this is a valid argument, to be honest.  Actually, the nature of this change is that it starts to parse strictly MORE cases.

> So I think this will cause issues for the suppression attribute you're working on if we allow it to have a GNU spelling with valid C89 code like; `__attribute__((suppress)) i = 12;`

This is **not** a valid code if you remove `__attribute__((suppress))`. `i = 12` without a prior declaration of `i` will cause the `use of undeclared identifier 'i'` error.  You can try it with any compiler: https://godbolt.org/z/P43nxn.  
I honestly don't see a scenario when the user has some piece of code that is parsed as a statement, then they mindfully add a //statement// attribute, and expect it to be parsed as a declaration after that.

> or `__attribute__((suppress)) g() { return 12; }` which may be intended to suppress the "type specifier missing" diagnostic (somewhat amusingly, since the user could also just add the `int` to the declaration since they're changing the code anyway).

Again, we **do not force** the parser to parse whatever follows the statement attribute as a statement.  `__attribute__((suppress))` will not change how we parse things here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93630