[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan added a comment.

In D121283#3392137 , @thakis wrote:

> The commit message got a bit mutilated: 
> 33a9eac6aaa495fce6fd9b17cd48aa57a95461e6 
> 
>
> Just FYI in case you didn't notice. In that case, update your commit workflow 
> to make sure this doesn't happen next time :)

That's strange, I didn't notice this, thanks for the heads up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The commit message got a bit mutilated: 
33a9eac6aaa495fce6fd9b17cd48aa57a95461e6 


Just FYI in case you didn't notice. In that case, update your commit workflow 
to make sure this doesn't happen next time :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG33a9eac6aaa4: [Clang] Support multiple attributes in a 
single pragma (authored by egorzhdan).

Changed prior to commit:
  https://reviews.llvm.org/D121283?vs=416279=416469#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp

Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
@@ -210,3 +207,13 @@
 #pragma clang attribute push([[clang::uninitialized]], apply_to=any) // expected-error {{expected '('}}
 #pragma clang attribute push([[clang::uninitialized]], apply_to = any()) // expected-error {{expected an identifier that corresponds to an attribute subject rule}}
 // NB: neither of these need to be popped; they were never successfully pushed.
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls,)), apply_to = function) // expected-error {{expected identifier that represents an attribute name}}
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,36 +48,39 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute 

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan updated this revision to Diff 416279.
egorzhdan added a comment.

- Fix off-by-one error in fix-it generation logic: the last SubjectMatchRule

(`SubjectMatchRule_variable_not_is_parameter`) was not handled properly

- Add a test for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp

Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
@@ -210,3 +207,13 @@
 #pragma clang attribute push([[clang::uninitialized]], apply_to=any) // expected-error {{expected '('}}
 #pragma clang attribute push([[clang::uninitialized]], apply_to = any()) // expected-error {{expected an identifier that corresponds to an attribute subject rule}}
 // NB: neither of these need to be popped; they were never successfully pushed.
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls,)), apply_to = function) // expected-error {{expected identifier that represents an attribute name}}
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,36 +48,39 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) (function))
-// CHECK: 

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan added a comment.

Thanks for the review @aaron.ballman!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan updated this revision to Diff 415527.
egorzhdan added a comment.

Merge two test files into one to speed up testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp

Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
@@ -210,3 +207,13 @@
 #pragma clang attribute push([[clang::uninitialized]], apply_to=any) // expected-error {{expected '('}}
 #pragma clang attribute push([[clang::uninitialized]], apply_to = any()) // expected-error {{expected an identifier that corresponds to an attribute subject rule}}
 // NB: neither of these need to be popped; they were never successfully pushed.
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls,)), apply_to = function) // expected-error {{expected identifier that represents an attribute name}}
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,35 +48,35 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) (function))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:71}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: 

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from a minor testing nit, this LGTM. Thank you for the new functionality!




Comment at: clang/test/Parser/pragma-multiple-attributes.cpp:1-11
+// RUN: %clang_cc1 -Wno-pragma-clang-attribute -verify %s
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], 
apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, 
annotate("test"))), apply_to = function)
+#pragma clang attribute pop

I think this entire test file can now be folded back into 
`pragma-attribute.cpp` (this way we don't have to pay the overhead of another 
execution of Clang to test the functionality).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan updated this revision to Diff 415199.
egorzhdan added a comment.

- Only allow one attribute syntax style per directive
- Adjust documentation
- Add a release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp
  clang/test/Parser/pragma-multiple-attributes.cpp

Index: clang/test/Parser/pragma-multiple-attributes.cpp
===
--- /dev/null
+++ clang/test/Parser/pragma-multiple-attributes.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -Wno-pragma-clang-attribute -verify %s
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls,)), apply_to = function) // expected-error {{expected identifier that represents an attribute name}}
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,35 +48,35 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) (function))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:71}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: 

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan added a comment.

In D121283#3379225 , @aaron.ballman 
wrote:

> As a thought experiment, would it make sense to lift the restriction on the 
> number of attributes allowed in a pragma, but not allow multiple attribute 
> specifiers?

I think this is reasonable, and as you've mentioned, it also leaves the 
opportunity to extend this syntax later should the need arise.
I will adjust this patch shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D121283#3375806 , @egorzhdan wrote:

> In D121283#3373560 , @aaron.ballman 
> wrote:
>
>> why do we support multiple attribute *specifiers* in the same pragma? I 
>> would not expect to be able to mix attribute styles in the same pragma given 
>> that all of the individual styles allow you to specify multiple attributes 
>> within a single specifier
>
> I don't think I have a strong use case for this. It seemed consistent with 
> the way multiple attributes can be added for individual declarations, e.g. 
> `__attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f()`. But we 
> can prohibit multiple specifiers within a single pragma if you think that 
> this is not a good construct to support.

I don't yet think it's a *bad* construct to support...

> In D121283#3373560 , @aaron.ballman 
> wrote:
>
>> why is whitespace the correct separator as opposed to a comma-delimited list?
>
> My motivation for this was also consistency with the syntax for attributes in 
> individual declarations. Given that attribute specifiers are delimited by 
> space for individual declarations (`__attribute__((cdecl)) 
> __attribute__((noreturn)) void f()`), I think it would be unintuitive to 
> require commas for attribute specifiers in pragmas when we could instead 
> reuse the existing syntax of space-delimited attribute specifiers.

Thanks, that makes some sense to me, but at the same time, I can't think of 
another pragma that behaves similarly off the top of my head (usually, lists of 
things have a delimiter other than whitespace), so it's kind of unintuitive 
either way.

As a thought experiment, would it make sense to lift the restriction on the 
number of attributes allowed in a pragma, but not allow multiple attribute 
specifiers? e.g.,

  // These are fine
  #pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], 
apply_to = function)
  #pragma clang attribute pop
  
  #pragma clang attribute push (__attribute__((noreturn, noinline)), apply_to = 
function)
  #pragma clang attribute pop
  
  // These are not allowed
  #pragma clang attribute push ([[clang::disable_tail_calls]] [[noreturn]], 
apply_to = function)
  #pragma clang attribute pop
  
  #pragma clang attribute push (__attribute__((noreturn)) 
__attribute__((noinline)), apply_to = function)
  #pragma clang attribute pop
  
  #pragma clang attribute push ([[clang::disable_tail_calls]] 
__attribute__((noreturn)), apply_to = function)
  #pragma clang attribute pop

This still allows you to specify more than one attribute in the pragma, but 
removes the concerns about how to separate the syntaxes (whitespace or another 
delimiter) while still leaving that design open in the future if there's a 
strong need to mix syntaxes.

The pragma attribute feature has a lot of power to it, but it also comes with a 
lot of risk to users, so it's a feature that I think we should be cautious 
about extending (in general). I think what you propose here could very well be 
reasonable, but I'm a bit worried that there's not a clear need for that amount 
of flexibility, which is why I'm sort of leaning towards being more restrictive 
here. However, this isn't exposing any functionality that users can't already 
do the long way with multiple pragmas, so I don't see a blocking concern with 
the current patch either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan added a comment.

In D121283#3373560 , @aaron.ballman 
wrote:

> why do we support multiple attribute *specifiers* in the same pragma? I would 
> not expect to be able to mix attribute styles in the same pragma given that 
> all of the individual styles allow you to specify multiple attributes within 
> a single specifier

I don't think I have a strong use case for this. It seemed consistent with the 
way multiple attributes can be added for individual declarations, e.g. 
`__attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f()`. But we 
can prohibit multiple specifiers within a single pragma if you think that this 
is not a good construct to support.

In D121283#3373560 , @aaron.ballman 
wrote:

> why is whitespace the correct separator as opposed to a comma-delimited list?

My motivation for this was also consistency with the syntax for attributes in 
individual declarations. Given that attribute specifiers are delimited by space 
for individual declarations (`__attribute__((cdecl)) __attribute__((noreturn)) 
void f()`), I think it would be unintuitive to require commas for attribute 
specifiers in pragmas when we could instead reuse the existing syntax of 
space-delimited attribute specifiers.

In D121283#3373560 , @aaron.ballman 
wrote:

> Also, I'd expect there to be some documentation changes along with this patch 
> and a release note.

Good point, thanks, I will add those to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for this, I plan on giving it a more thorough review when I can. But the 
summary led me to a design question: why do we support multiple attribute 
*specifiers* in the same pragma? I would not expect to be able to mix attribute 
styles in the same pragma given that all of the individual styles allow you to 
specify multiple attributes within a single specifier. e.g., 
https://godbolt.org/z/9v7r6Eanz

  __declspec(deprecated, naked) void f();
  __attribute__((annotate("test"), constructor(0))) void g();
  [[clang::annotate("test"), gnu::constructor(0)]] void h();

What's the use case for letting someone mix and match attribute styles as in:

  #pragma clang attribute push ([[clang::disable_tail_calls]] 
__attribute__((annotate("test"))), apply_to = function)

and why is whitespace the correct separator as opposed to a comma-delimited 
list?

(Note, I'm still thinking about whether this approach poses any actual 
problems; it may be perfectly fine, but it'd help to know why we're being lax 
in mixing forms and what the rationale is for whitespace separation; that's not 
a common approach to lists of things in C or C++).

Also, I'd expect there to be some documentation changes along with this patch 
and a release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

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

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan updated this revision to Diff 414068.
egorzhdan added a comment.

Remove unused include


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121283

Files:
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp
  clang/test/Parser/pragma-multiple-attributes.cpp

Index: clang/test/Parser/pragma-multiple-attributes.cpp
===
--- /dev/null
+++ clang/test/Parser/pragma-multiple-attributes.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -Wno-pragma-clang-attribute -verify %s
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls)) __attribute__((annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
+
+#pragma clang attribute push (test [[noreturn]]) // expected-error {{expected an attribute that is specified using the GNU, C++11 or '__declspec' syntax}}
+
+#pragma clang attribute push ([[noreturn]] test) // expected-error {{expected ',' or an attribute that is specified using the GNU, C++11 or '__declspec' syntax}}
Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,35 +48,33 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
-#pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) (function))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:71}:", apply_to = any(record(unless(is_union)), variable, 

[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Egor Zhdan via Phabricator via cfe-commits]

egorzhdan created this revision.
egorzhdan added a reviewer: arphaman.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
egorzhdan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This adds support for multiple attributes in `#pragma clang attribute push`, 
for example:

  #pragma clang attribute push 
(__attribute__((disable_sanitizer_instrumentation)) 
__attribute__((section("S"))), apply_to=variable(is_global))

or

  #pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], 
apply_to = function)

Related attributes can now be applied with a single pragma, which makes it 
harder for developers to make an accidental error later when editing the code.

rdar://78269653


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121283

Files:
  clang/include/clang/Basic/AttrSubjectMatchRules.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/test/AST/pragma-multiple-attributes-declspec.cpp
  clang/test/AST/pragma-multiple-attributes.cpp
  clang/test/FixIt/fixit-pragma-attribute.c
  clang/test/FixIt/fixit-pragma-attribute.cpp
  clang/test/Parser/pragma-attribute-declspec.cpp
  clang/test/Parser/pragma-attribute.cpp
  clang/test/Parser/pragma-multiple-attributes.cpp

Index: clang/test/Parser/pragma-multiple-attributes.cpp
===
--- /dev/null
+++ clang/test/Parser/pragma-multiple-attributes.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -Wno-pragma-clang-attribute -verify %s
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls)) __attribute__((annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}
+
+#pragma clang attribute push (test [[noreturn]]) // expected-error {{expected an attribute that is specified using the GNU, C++11 or '__declspec' syntax}}
+
+#pragma clang attribute push ([[noreturn]] test) // expected-error {{expected ',' or an attribute that is specified using the GNU, C++11 or '__declspec' syntax}}
Index: clang/test/Parser/pragma-attribute.cpp
===
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
Index: clang/test/Parser/pragma-attribute-declspec.cpp
===
--- clang/test/Parser/pragma-attribute-declspec.cpp
+++ clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
Index: clang/test/FixIt/fixit-pragma-attribute.cpp
===
--- clang/test/FixIt/fixit-pragma-attribute.cpp
+++ clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
@@ -48,35 +48,33 @@
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: