-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[clang] Reject 'auto' storage class with type specifier in C++ #166004
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang Author: Osama Abdelkader (osamakader) ChangesFixes #164273 Full diff: https://github.com/llvm/llvm-project/pull/166004.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index e5e071f43fa75..baf91b107b8c4 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -399,6 +399,8 @@ def err_requires_clause_on_declarator_not_declaring_a_function : Error<
"trailing requires clause can only be used when declaring a function">;
def err_requires_clause_inside_parens : Error<
"trailing requires clause should be placed outside parentheses">;
+def err_auto_type_specifier : Error<
+ "'auto' cannot be combined with a type specifier in C++">;
def ext_auto_storage_class : ExtWarn<
"'auto' storage class specifier is not permitted in C++11, and will not "
"be supported in future releases">, InGroup<DiagGroup<"auto-storage-class">>;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7e4a164e34eda..0044201b459b3 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -4095,11 +4095,17 @@ void Parser::ParseDeclarationSpecifiers(
case tok::kw_auto:
if (getLangOpts().CPlusPlus11 || getLangOpts().C23) {
if (isKnownToBeTypeSpecifier(GetLookAheadToken(1))) {
- isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_auto, Loc,
- PrevSpec, DiagID, Policy);
- if (!isInvalid && !getLangOpts().C23)
- Diag(Tok, diag::ext_auto_storage_class)
- << FixItHint::CreateRemoval(DS.getStorageClassSpecLoc());
+ // In C++ (not C23), 'auto' cannot be combined with a type specifier.
+ if (getLangOpts().CPlusPlus && !getLangOpts().C23) {
+ isInvalid = true;
+ if (!PrevSpec)
+ PrevSpec = "";
+ DiagID = diag::err_auto_type_specifier;
+ } else {
+ // C23 allows 'auto' as storage class with type specifier.
+ isInvalid = DS.SetStorageClassSpec(Actions, DeclSpec::SCS_auto, Loc,
+ PrevSpec, DiagID, Policy);
+ }
} else
isInvalid = DS.SetTypeSpecType(DeclSpec::TST_auto, Loc, PrevSpec,
DiagID, Policy);
diff --git a/clang/test/Parser/cxx-auto-type-specifier.cpp b/clang/test/Parser/cxx-auto-type-specifier.cpp
new file mode 100644
index 0000000000000..7a1291b08f7d3
--- /dev/null
+++ b/clang/test/Parser/cxx-auto-type-specifier.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++14 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++23 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=c23 -std=c23 %s
+
+// Test that 'auto' cannot be combined with a type specifier in C++.
+void f() {
+ auto int x = 1; // expected-error {{'auto' cannot be combined with a type specifier in C++}}
+ auto char c = 'a'; // expected-error {{'auto' cannot be combined with a type specifier in C++}}
+ auto float f = 1.0f; // expected-error {{'auto' cannot be combined with a type specifier in C++}}
+ auto double d = 1.0; // expected-error {{'auto' cannot be combined with a type specifier in C++}}
+ auto long l = 1L; // expected-error {{'auto' cannot be combined with a type specifier in C++}}
+}
+
+// Test that 'auto' as a storage class with type specifier is still allowed in C23.
+void g() {
+ auto int x = 1; // c23-no-diagnostics
+ auto char c = 'a'; // c23-no-diagnostics
+}
+
+// Test that regular 'auto' (type deduction) still works in C++.
+void h() {
+ auto x = 1; // expected-no-diagnostics
+ auto y = 2.0; // expected-no-diagnostics
+ auto z = 'c'; // expected-no-diagnostics
+}
+
|
46b862a to
9bc6045
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
b0d5dd5 to
eb5b66d
Compare
|
I am going to be at the wg21 meeting next week, so I may not have a chance to look at this. |
eb5b66d to
649ca40
Compare
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.
Changes to C++ DR tests LGTM otherwise.
19c937e to
42ab668
Compare
| def err_requires_clause_inside_parens : Error< | ||
| "trailing requires clause should be placed outside parentheses">; | ||
| def err_auto_type_specifier : Error< | ||
| "'auto' cannot be combined with a type specifier in C++">; |
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.
I would drop the " in C++" here. That's not actually true -- auto can be combined with a type specifier in C++98. (And saying " in C++11 onwards" or similar seems unnecessary these days.)
clang/lib/Parse/ParseDecl.cpp
Outdated
| << FixItHint::CreateRemoval(DS.getStorageClassSpecLoc()); | ||
| // In C++ (not C23), 'auto' cannot be combined with a type specifier. | ||
| // However, OpenCL has its own error handling for this case. | ||
| if (getLangOpts().CPlusPlus && !getLangOpts().C23 && |
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.
No need to check both CPlusPlus and C23 here; they're mutually exclusive.
clang/lib/Parse/ParseDecl.cpp
Outdated
| Diag(Tok, diag::ext_auto_storage_class) | ||
| << FixItHint::CreateRemoval(DS.getStorageClassSpecLoc()); | ||
| // In C++ (not C23), 'auto' cannot be combined with a type specifier. | ||
| // However, OpenCL has its own error handling for this case. |
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.
What is the OpenCL handling? If it's the same as the general C++ handling, can we remove the OpenCL handling and enable the new code for OpenCL too?
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.
Without excluding OpenCL, the parser emitted our generic error "'auto' cannot be combined with a type specifier" instead of the OpenCL-specific one.
The test clang/test/SemaOpenCL/storageclass.cl expects:
expected-error{{C++ for OpenCL version 2021 does not support the 'auto' storage class specifier}}
or for OpenCL C:
expected-error-re{{OpenCL C version {{1.2|3.0}} does not support the 'auto' storage class specifier}}
If we don't exclude OpenCL, the parser emits our error and the test fails because it expects the OpenCL-specific message. Excluding OpenCL (!getLangOpts().OpenCLCPlusPlus) lets Sema emit the OpenCL-specific error.
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 just remove the OpenCL-specific diagnostic from the OpenCL code and emit the diagnostic for both C++ and OpenCL here instead?
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.
cc @svenvh WDYT?
8473ae6 to
711223c
Compare
Previously, clang allowed 'auto int x = 1;' in C++ as an extension (for C compatibility), emitting only a warning. This was confusing since 'auto' in C++11+ is a type specifier, not a storage class. This patch: - Adds a new error diagnostic 'err_auto_type_specifier' - Updates the parser to emit an error (instead of warning) when 'auto' is used as a storage class with a type specifier in C++ mode - Preserves C23 behavior where 'auto int' is valid - Adds comprehensive tests Fixes llvm#164273 Signed-off-by: Osama Abdelkader <[email protected]>
711223c to
849e398
Compare
| def ext_auto_storage_class : ExtWarn< | ||
| "'auto' storage class specifier is not permitted in C++11, and will not " |
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 pr makes ext_auto_storage_class unused, so we should remove it
| // redundant error (GCC only emits one error for this case: "two or | ||
| // more data types in declaration"). | ||
| isInvalid = true; | ||
| PrevSpec = "auto"; |
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.
| PrevSpec = "auto"; | |
| PrevSpec = Tok.getIdentifierInfo()->getNameStart(); |
| // expected-error@-2 {{'auto' cannot be combined with a type specifier}} | ||
| #else | ||
| // expected-error@-4 {{illegal storage class on file-scoped variable}} |
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.
It'd be slightly easier to review this file if we used -verify= and different prefixes instead of preprocessor conditionals, but I don't insist on a change for this PR either unless others have strong feelings.
| void p3example() { | ||
| auto x = 5; | ||
| const auto *v = &x, u = 6; | ||
| static auto y = 0.0; |
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 line should also be diagnosed in C++98 mode... this seems to be a preexisting bug: https://godbolt.org/z/3cza618cx (we have a similar bug with register: https://godbolt.org/z/zs5918YY4)
GitHub is too useless to let me put this comment on the correct line, but line 40 has a similar C++98 bug: https://godbolt.org/z/xEozMhe6z because there's no type specifier there, and line 41 does as well for the same reason.
Hmmm, it seems we're treating type deduction as an extension in C++98 mode, but we don't list it in the language extensions page as such and we don't issue pedantic diagnostics for it. @zygoloid do you recall if this was an intentional extension or not? Regardless, because this is existing behavior, nothing needs to change as part of this PR.
| void f() { | ||
| auto int x = 1; // expected-error {{'auto' cannot be combined with a type specifier}} |
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.
One more test to add:
template <typename Ty>
void g() {
auto Ty x;
}
void test() {
g<float>();
}
Fixes #164273