vsavchenko added inline comments.
================ Comment at: clang/lib/Parse/ParseStmt.cpp:213 ParsedStmtContext()) && - (GNUAttributeLoc.isValid() || isDeclarationStatement())) { + ((GNUAttributeLoc.isValid() && !Attrs.back().isStmtAttr()) || + isDeclarationStatement())) { ---------------- aaron.ballman wrote: > 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. > 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 look at `clang/test/Sema/attr-nomerge.cpp`, we can see that we have exactly the same behavior as in C++ with `[[clang::nomerge]]`: ``` [[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute cannot be applied to a declaration}} ``` > 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. I checked that as well and for this example before this patch, after this patch, and in C++ when using `[[clang::nomerge]]` we get exactly the same behavior: 3 errors `'nomerge' attribute cannot be applied to a declaration`. 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