-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Clang][diagnostics] Fix structured binding shadows template param loc #129116
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][diagnostics] Fix structured binding shadows template param loc #129116
Conversation
@llvm/pr-subscribers-clang Author: Amr Hesham (AmrDeveloper) ChangesFix structured binding shadows template parameter location Fixes: #129060 Full diff: https://github.com/llvm/llvm-project/pull/129116.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 664d48ccbc382..a3a028b9485d6 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -883,8 +883,7 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D,
// It's not permitted to shadow a template parameter name.
if (Previous.isSingleResult() &&
Previous.getFoundDecl()->isTemplateParameter()) {
- DiagnoseTemplateParameterShadow(D.getIdentifierLoc(),
- Previous.getFoundDecl());
+ DiagnoseTemplateParameterShadow(B.NameLoc, Previous.getFoundDecl());
Previous.clear();
}
diff --git a/clang/test/CXX/temp/temp.res/temp.local/p6.cpp b/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
index 00bb35813c39a..e464bb5e7eaef 100644
--- a/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
+++ b/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
@@ -162,3 +162,8 @@ struct A {
};
A<0>::B a;
}
+
+template <typename T> void shadow9() { // expected-note{{template parameter is declared here}}
+ using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}}
+ auto [T] = arr{}; // expected-error {{declaration of 'T' shadows template parameter}}
+}
|
@shafik I added the sample to our test cases, not sure if we can add source loc in expected line, I will check that |
h/t @erichkeane something like this should help get us the confirmation in the test we want: https://godbolt.org/z/1GfK1K8qf |
|
||
template <typename T> void shadow9() { // expected-note{{template parameter is declared here}} | ||
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}} | ||
auto [T] = arr{}; // expected-error {{declaration of 'T' shadows template 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.
I think this test case would benefit from the new lines added in the initial issue report - the reported behavior is due to us choosing to report an error based on the wrong part of the declaration, and I don't believe that this test case is sufficient to highlight the problem (or more to the point catch a regression in the behavior)
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.
Perhaps it makes sense to also add a release note.
clang/docs/ReleaseNotes.rst
Outdated
@@ -214,6 +214,8 @@ Improvements to Clang's diagnostics | |||
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The | |||
feature will be default-enabled with ``-Wthread-safety`` in a future release. | |||
|
|||
- Improve the diagnostics for shadows template 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.
This should probably mention the bug# as well as slightly better clarify that it is giving a more correct location.
@@ -162,3 +162,10 @@ struct A { | |||
}; | |||
A<0>::B a; | |||
} | |||
|
|||
template <typename T> void shadow() { // expected-note{{template parameter is declared 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.
Please instantiate this template as well.
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.
You mean something like 'template void shadow2() { ... }` or creating a sample with function call
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.
Just a call to this function, so: shadow<int>()
call. If you change it to return int
, you can do: auto Use = shadow<int>()
below this to instantiate it.
@@ -162,3 +162,10 @@ struct A { | |||
}; | |||
A<0>::B a; | |||
} | |||
|
|||
template <typename T> void shadow() { // expected-note{{template parameter is declared here}} | |||
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}} |
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.
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}} | |
using arr = int[1]; | |
// expected-warning@+1 {{decomposition declarations are a C++17 extension}} |
llvm#129116) Fix structured binding shadows template parameter location Fixes: llvm#129060
Fix structured binding shadows template parameter location
Fixes: #129060