-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,12 @@ void p3example() { | |
| static auto y = 0.0; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see a diagnostic for it appearing in all of your test cases: ... and If we want that table to be complete, it's probably worth going through the diagnostics in the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ugh, yeah, I am seeing that now, I think I was looking at the wrong output before. :-D Yeah, I think the table just needs to be updated, that doesn't need to be a part of this PR though. Thanks! |
||
| // In C++98: 'auto' storage class specifier is redundant and incompatible with C++0x | ||
| // In C++0x: 'auto' storage class specifier is not permitted in C++0x, and will not be supported in future releases | ||
| auto int r; // expected-warning {{'auto' storage class specifier}} | ||
| auto int r; | ||
| #if __cplusplus >= 201103L | ||
| // expected-error@-2 {{'auto' cannot be combined with a type specifier}} | ||
| #else | ||
| // expected-warning@-4 {{'auto' storage class specifier}} | ||
| #endif | ||
|
|
||
| same<__typeof(x), int> xHasTypeInt; | ||
| same<__typeof(v), const int*> vHasTypeConstIntPtr; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // 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 | ||
|
|
||
| // 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}} | ||
|
Comment on lines
+8
to
+9
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more test to add:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Template-dependent types can't be detected at parse time AFAIK.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why there's an instantiation of the template. The point is mostly: we need to be sure we're rejecting that code even though it's a semantic error rather than a parsing error. It's possible that the better way to approach this is from
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can definitely detect that there's both an
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think: will need to be factored away, then. When I looked into this, the issue is that the token we get is not actually annotated because nothing calls a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I tried that: TryAnnotate* incorrectly annotates some syntax e.g. in llvm-project/clang/test/Parser/cxx-concepts-requires-clause.cpp: As true which breaks this test and many other tests.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What kind of test failures are you seeing, exactly? Btw, I think it should be Do we maybe also need to do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests e.g. fire the diagnostic message, false positives. Even with Going to try
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can someone clarify why we would need new annotation or tentative parsing here? It seems to me that we can handle
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't see what annotation we would need. For this example: We know |
||
| auto char c = 'a'; // expected-error {{'auto' cannot be combined with a type specifier}} | ||
| auto float f = 1.0f; // expected-error {{'auto' cannot be combined with a type specifier}} | ||
| auto double d = 1.0; // expected-error {{'auto' cannot be combined with a type specifier}} | ||
| auto long l = 1L; // expected-error {{'auto' cannot be combined with a type specifier}} | ||
| } | ||
|
|
||
| // Test that regular 'auto' (type deduction) still works in C++. | ||
| void h() { | ||
| auto x = 1; | ||
| auto y = 2.0; | ||
| auto z = '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.
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.