aaron.ballman added a comment.

In D93630#2470048 <https://reviews.llvm.org/D93630#2470048>, @vsavchenko wrote:

> In D93630#2468853 <https://reviews.llvm.org/D93630#2468853>, @aaron.ballman 
> wrote:
>
>> Yeah, I kind of figured that might be the cause. I'm not 100% convinced (one 
>> way or the other) if the suppress attribute should get a GNU spelling. The 
>> `[[]]` spellings are available in all language modes (we have 
>> `-fdouble-square-bracket-attributes` to enable this) and don't run afoul of 
>> the "guess what this attribute appertains to" problem that GNU-style 
>> attributes do.
>
> I don't think that we can force our users into adding this flag.

You don't have to -- ObjC could always imply the flag is set (and I imagine 
ObjC will eventually update to C23 as the base language, where it'll be 
supported anyway).

> Also, Obj-C codebases already use a good amount of GNU-style attributes, so 
> it is pretty natural there.

IIRC, those attributes are often hidden behind macros, so I'm not certain how 
critical this is if the `-fdouble-square-bracket-attributes` feature was 
enabled in ObjC mode.



================
Comment at: clang/lib/Parse/ParseStmt.cpp:213
              ParsedStmtContext()) &&
-        (GNUAttributeLoc.isValid() || isDeclarationStatement())) {
+        ((GNUAttributeLoc.isValid() && !Attrs.back().isStmtAttr()) ||
+         isDeclarationStatement())) {
----------------
vsavchenko wrote:
> aaron.ballman wrote:
> > I think you need to ensure there's at least one attribute before checking 
> > `!Attrs.back().isStmtAttr()` as this may cause problems if the user does 
> > something odd like `__attribute__(()) int x;` (I don't know if this will 
> > result in a valid `GNUAttributeLoc` with no attributes or not.)
> > 
> > I'm not certain that logic is perfect either. It would be pretty mysterious 
> > to handle these cases differently:
> > ```
> > __attribute__((stmt_attr, decl_attr)) int a, b, c;
> > __attribute__((decl_attr, stmt_attr)) int x, y, z;
> > ```
> Yep, my bad.  I changed it so that ALL the attributes in the front should be 
> statement attributes.
I think this is a better approach but it still doesn't leave us a clean way to 
handle attributes that are both declaration and statement attributes. I'm 
nervous about this in the same way I'm nervous about GNU attributes sliding 
around in general -- it doesn't always work out the way a user might expect.

`nomerge` is a good example. You currently cannot write this code:
```
int f(void);

void func(void) {
  __attribute__((nomerge)) static int i = f();
}
```
because of the way GNU attributes start a declaration. However, I think this 
could also be considered a bug because `nomerge` on a statement (which this 
also is) will ensure that the call expression to `f()` is not merged during 
optimization.

If we now say "GNU attribute can now be used at the start of an arbitrary 
statement", we have to late parse the attribute until we know what kind of 
statement we have. Because the user could have written:
```
void func(void) {
  __attribute__((nomerge)) extern void f(void), g(int), h(void);
}
```
which is currently accepted today and would change meaning if we treated 
`nomerge` as a statement attribute.


================
Comment at: clang/test/Parser/stmt-attributes.c:89
+
+__attribute__((nomerge)) static int i; // expected-error {{'nomerge' attribute 
cannot be applied to a declaration}}
----------------
This is a semi-good example of the kind of behavior I was worried about -- 
`nomerge` can be applied to a declaration or a statement, just not *this* kind 
of declaration, and so the diagnostic is confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93630

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to