-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang] disallow attributes on void parameters #124920
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #108819 This PR introduces diagnostics to disallow the use of attributes on void parameters void f([[deprecated]] void) {} Full diff: https://github.com/llvm/llvm-project/pull/124920.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7fafe2807bd388..0c87e52007d546 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -100,6 +100,8 @@ Removed Compiler Flags
Attribute Changes in Clang
--------------------------
+- Clang now disallows the use of attributes on void parameters. (#GH108819)
+
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index f136d5007e8a5f..0b88dd4449b1e2 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -7986,6 +7986,13 @@ void Parser::ParseParameterDeclarationClause(
if (getLangOpts().HLSL)
MaybeParseHLSLAnnotations(DS.getAttributes());
+ if (ParmDeclarator.getIdentifier() == nullptr &&
+ ParmDeclarator.getDeclarationAttributes().size() &&
+ ParmDeclarator.getDeclSpec().getTypeSpecType() == DeclSpec::TST_void) {
+ SourceRange AttrRange = ParmDeclarator.getDeclarationAttributes().Range;
+ Diag(AttrRange.getBegin(), diag::err_attributes_not_allowed) << AttrRange;
+ }
+
if (Tok.is(tok::kw_requires)) {
// User tried to define a requires clause in a parameter declaration,
// which is surely not a function declaration.
diff --git a/clang/test/Parser/cxx0x-attributes.cpp b/clang/test/Parser/cxx0x-attributes.cpp
index fad3010c98b9c2..13fcdd142fa841 100644
--- a/clang/test/Parser/cxx0x-attributes.cpp
+++ b/clang/test/Parser/cxx0x-attributes.cpp
@@ -453,3 +453,12 @@ namespace P2361 {
}
alignas(int) struct AlignAsAttribute {}; // expected-error {{misplaced attributes; expected attributes here}}
+
+namespace GH108819 {
+void a([[maybe_unused]] void) {} // expected-error {{an attribute list cannot appear here}} \
+ // expected-warning {{use of the 'maybe_unused' attribute is a C++17 extension}}
+void b([[deprecated]] void) {} // expected-error {{an attribute list cannot appear here}} \
+ // expected-warning {{use of the 'deprecated' attribute is a C++14 extension}}
+void c([[clang::lifetimebound]] void) {} // expected-error {{an attribute list cannot appear here}}
+void d([[clang::annotate("a", "b", 1)]] void) {} // expected-error {{an attribute list cannot appear here}}
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 nit, else LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a semantic restriction, not a parsing restriction, right?
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense but we should have been coverage in testing.
@@ -53,3 +53,14 @@ alignas(4) auto PR19252 = 0; | |||
|
|||
// Check the diagnostic message | |||
class alignas(void) AlignasVoid {}; // expected-error {{invalid application of 'alignas' to an incomplete type 'void'}} | |||
|
|||
namespace GH108819 { | |||
void a([[maybe_unused]] void) {} // expected-warning {{attribute 'maybe_unused' cannot be applied to a 'void' parameter}}\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test for a member function. also a templated function, lambdas as well. Other combination if you can come up w/ them should also be tested. We need to do a better job of covering as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shafik Thanks for the feedback. I've added more test cases. Could you take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #108819
This PR introduces diagnostics to disallow the use of attributes on void parameters