[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
smeenai added a comment. Herald added a project: All. This is very belated, sorry :) Our shadowing warnings have special behavior for lambdas and local variables: the warning for lambdas shadowing uncaptured local variables falls under `-Wshadow-uncaptured-local`, which isn't turned on for `-Wshadow` (only for `-Wshadow-all`). However, this doesn't extend to structured bindings. Compare https://godbolt.org/z/nP6b9f8o4, which doesn't fire for `-Wshadow`, to https://godbolt.org/z/9he8Ezsbb, which is the equivalent with structured bindings and does fire for `-Wshadow`. This is triggering a whole bunch of new `-Wshadow` warnings in our codebase, and I'm wondering if we'd consider also separating the lambda + structured binding case into its own warning group, to match the behavior for lambdas + local variables. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
This revision was automatically updated to reflect the committed changes.
Closed by commit rG039f79c78cfa: [SEMA] Added warn_decl_shadow support for
structured bindings (authored by Vexthil, committed by rsmith).
Changed prior to commit:
https://reviews.llvm.org/D96147?vs=325578=325892#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,44 @@
PR24718_1 // Does not shadow a type.
};
};
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+struct S {
+ int a, b;
+};
+
+void test1() {
+ const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = S(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = S(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+void test4() {
+ const auto [a, b] = S(); // expected-note 3 {{previous declaration is here}}
+ {
+int a = 4; // expected-warning {{declaration shadows a structured binding}}
+ }
+ {
+const auto [a, b] = S(); // expected-warning 2 {{declaration shadows a structured binding}}
+ }
+}
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,25 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
+} else if (ShadowedDecl && !D.isRedeclaration()) {
+ CheckShadow(BD, ShadowedDecl, Previous);
}
-
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7491,7 +7491,8 @@
SDK_StaticMember,
SDK_Field,
SDK_Typedef,
- SDK_Using
+ SDK_Using,
+ SDK_StructuredBinding
};
/// Determine what kind of declaration we're shadowing.
@@ -7501,6 +7502,8 @@
return SDK_Using;
else if (isa(ShadowedDecl))
return SDK_Typedef;
+ else if (isa(ShadowedDecl))
+return SDK_StructuredBinding;
else if (isa(OldDC))
return isa(ShadowedDecl) ? SDK_Field : SDK_StaticMember;
@@ -7540,9 +7543,8 @@
return nullptr;
NamedDecl *ShadowedDecl = R.getFoundDecl();
- return isa(ShadowedDecl) || isa(ShadowedDecl)
- ? ShadowedDecl
- : nullptr;
+ return isa(ShadowedDecl) ? ShadowedDecl
+: nullptr;
}
/// Return the declaration shadowed by the given typedef \p D, or null
@@ -7560,6 +7562,18 @@
return isa(ShadowedDecl) ? ShadowedDecl : nullptr;
}
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil added a comment. .. And yes I will need someone to commit this for me I believe once you are happy with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil updated this revision to Diff 325578.
Vexthil added a comment.
Added unit tests for "shadows a structured binding"
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,44 @@
PR24718_1 // Does not shadow a type.
};
};
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+struct S {
+ int a, b;
+};
+
+void test1() {
+ const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = S(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = S(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+void test4() {
+ const auto [a, b] = S(); // expected-note 3 {{previous declaration is here}}
+ {
+int a = 4; // expected-warning {{declaration shadows a structured binding}}
+ }
+ {
+const auto [a, b] = S(); // expected-warning 2 {{declaration shadows a structured binding}}
+ }
+}
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,25 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
+} else if (ShadowedDecl && !D.isRedeclaration()) {
+ CheckShadow(BD, ShadowedDecl, Previous);
}
-
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7491,7 +7491,8 @@
SDK_StaticMember,
SDK_Field,
SDK_Typedef,
- SDK_Using
+ SDK_Using,
+ SDK_StructuredBinding
};
/// Determine what kind of declaration we're shadowing.
@@ -7501,6 +7502,8 @@
return SDK_Using;
else if (isa(ShadowedDecl))
return SDK_Typedef;
+ else if (isa(ShadowedDecl))
+return SDK_StructuredBinding;
else if (isa(OldDC))
return isa(ShadowedDecl) ? SDK_Field : SDK_StaticMember;
@@ -7540,9 +7543,8 @@
return nullptr;
NamedDecl *ShadowedDecl = R.getFoundDecl();
- return isa(ShadowedDecl) || isa(ShadowedDecl)
- ? ShadowedDecl
- : nullptr;
+ return isa(ShadowedDecl) ? ShadowedDecl
+: nullptr;
}
/// Return the declaration shadowed by the given typedef \p D, or null
@@ -7560,6 +7562,18 @@
return isa(ShadowedDecl) ? ShadowedDecl : nullptr;
}
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+const LookupResult ) {
+ if (!shouldWarnIfShadowedDecl(Diags, R))
+return nullptr;
+
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa(ShadowedDecl) ? ShadowedDecl
+
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
rsmith added a comment. Hm, before we go forward with this: can we get a test for the "shadows a structured binding" case, please? You might also need to add a new case to the "declaration shadows ..." warning text. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good, thanks! Do you need someone to commit this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil updated this revision to Diff 325256.
Vexthil added a comment.
Fixing clang format issues. The SemaDecl.cpp has a pile of incorrect clang
format issues but i've only updated changes relevant to changes I have made
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,34 @@
PR24718_1 // Does not shadow a type.
};
};
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+struct S {
+ int a, b;
+};
+
+void test1() {
+ const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = S(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = S(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,25 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
+} else if (ShadowedDecl && !D.isRedeclaration()) {
+ CheckShadow(BD, ShadowedDecl, Previous);
}
-
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7540,9 +7540,8 @@
return nullptr;
NamedDecl *ShadowedDecl = R.getFoundDecl();
- return isa(ShadowedDecl) || isa(ShadowedDecl)
- ? ShadowedDecl
- : nullptr;
+ return isa(ShadowedDecl) ? ShadowedDecl
+: nullptr;
}
/// Return the declaration shadowed by the given typedef \p D, or null
@@ -7560,6 +7559,18 @@
return isa(ShadowedDecl) ? ShadowedDecl : nullptr;
}
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+const LookupResult ) {
+ if (!shouldWarnIfShadowedDecl(Diags, R))
+return nullptr;
+
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa(ShadowedDecl) ? ShadowedDecl
+: nullptr;
+}
+
/// Diagnose variable or built-in function shadowing. Implements
/// -Wshadow.
///
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2598,6 +2598,8 @@
NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D,
const LookupResult );
NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult );
+ NamedDecl *getShadowedDeclaration(const BindingDecl *D,
+const
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil updated this revision to Diff 325252.
Vexthil added a comment.
Update with changes suggested by @rsmith .
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,34 @@
PR24718_1 // Does not shadow a type.
};
};
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+struct S {
+ int a, b;
+};
+
+void test1() {
+ const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = S(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = S(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,25 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
+} else if (ShadowedDecl && !D.isRedeclaration()) {
+ CheckShadow(BD, ShadowedDecl, Previous);
}
-
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7540,7 +7540,7 @@
return nullptr;
NamedDecl *ShadowedDecl = R.getFoundDecl();
- return isa(ShadowedDecl) || isa(ShadowedDecl)
+ return isa(ShadowedDecl)
? ShadowedDecl
: nullptr;
}
@@ -7560,6 +7560,19 @@
return isa(ShadowedDecl) ? ShadowedDecl : nullptr;
}
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+const LookupResult ) {
+ if (!shouldWarnIfShadowedDecl(Diags, R))
+return nullptr;
+
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa(ShadowedDecl)
+ ? ShadowedDecl
+ : nullptr;
+}
+
/// Diagnose variable or built-in function shadowing. Implements
/// -Wshadow.
///
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2598,6 +2598,8 @@
NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D,
const LookupResult );
NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult );
+ NamedDecl *getShadowedDeclaration(const BindingDecl *D,
+const LookupResult );
void CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
const LookupResult );
void CheckShadow(Scope *S, VarDecl *D);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
rsmith added a comment.
Thanks!
Comment at: clang/lib/Sema/SemaDecl.cpp:7571
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa(ShadowedDecl) || isa(ShadowedDecl)
+ ? ShadowedDecl
rsmith wrote:
> I think we should also warn if a `BindingDecl` shadows another `BindingDecl`,
> or if a `VarDecl` shadows a `BindingDecl`.
`isa(ShadowedDecl) || isa(ShadowedDecl)` can be simplified
to `isa(ShadowedDecl)`.
Comment at: clang/lib/Sema/SemaDecl.cpp:7571-7573
+ return isa(ShadowedDecl) || isa(ShadowedDecl)
+ ? ShadowedDecl
+ : nullptr;
I think we should also warn if a `BindingDecl` shadows another `BindingDecl`,
or if a `VarDecl` shadows a `BindingDecl`.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:872-874
+// Diagnose shadowed variables if this isn't a redeclaration.
+if (ShadowedDecl && !D.isRedeclaration())
+ CheckShadow(BD, ShadowedDecl, Previous);
Should this be an `else` for the `if (!Previous.empty())` below? Do we get two
diagnostics for:
```
int a;
struct X { int n; };
auto [a] = X();
```
(one for shadowing and one for redefinition)?
Comment at: clang/test/SemaCXX/warn-shadow.cpp:274
+#ifndef USE_STD
+// Machinery required for custom structured bindings decomposition.
+typedef unsigned long size_t;
It doesn't seem important to test different kinds of bindings here, since the
shadowing check for bindings doesn't depend on how we perform the
decomposition. So I'd suggest you simplify this test by using only built-in
bindings, eg:
```
namespace structured_binding_tests {
int x; // expected-note {{previous declaration is here}}
int y; // expected-note {{previous declaration is here}}
struct S { int a, b; };
void test1() {
const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a
variable in namespace 'structured_binding_tests'}}
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil updated this revision to Diff 321930.
Vexthil added a comment.
Fixed the update by doing a full diff rather than just the additive update
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96147/new/
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,86 @@
PR24718_1 // Does not shadow a type.
};
};
+
+// MyTuple and std code is copied from live-bindings-test.cpp
+
+//#define USE_STD
+
+#ifndef USE_STD
+// Machinery required for custom structured bindings decomposition.
+typedef unsigned long size_t;
+
+namespace std {
+template class tuple_size;
+template
+constexpr size_t tuple_size_v = tuple_size::value;
+template class tuple_element;
+
+template
+struct integral_constant {
+ static constexpr T value = v;
+ typedef T value_type;
+ typedef integral_constant type;
+ constexpr operator value_type() const noexcept { return value; }
+};
+} // namespace std
+
+struct MyTuple {
+ int a;
+ int b;
+
+ template
+ int get() const {
+if constexpr (N == 0)
+ return a;
+else if constexpr (N == 1)
+ return b;
+ }
+};
+
+namespace std {
+template <>
+struct tuple_size
+: std::integral_constant {};
+
+template
+struct tuple_element {
+ using type = int;
+};
+} // namespace std
+
+MyTuple getMyTuple();
+#else
+
+#include
+std::tuple getMyTuple();
+#endif
+
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+
+void test1() {
+ const auto [x, y] = getMyTuple(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = getMyTuple(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = getMyTuple(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,28 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
+// Diagnose shadowed variables if this isn't a redeclaration.
+if (ShadowedDecl && !D.isRedeclaration())
+ CheckShadow(BD, ShadowedDecl, Previous);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
}
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7560,6 +7560,19 @@
return isa(ShadowedDecl) ? ShadowedDecl : nullptr;
}
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+const LookupResult ) {
+ if (!shouldWarnIfShadowedDecl(Diags, R))
+return nullptr;
+
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa(ShadowedDecl) || isa(ShadowedDecl)
+ ? ShadowedDecl
+ : nullptr;
+}
+
/// Diagnose variable or built-in function shadowing.
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil updated this revision to Diff 321860. Vexthil added a comment. Updating clang format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 Files: clang/include/clang/Sema/Sema.h Index: clang/include/clang/Sema/Sema.h === --- clang/include/clang/Sema/Sema.h +++ clang/include/clang/Sema/Sema.h @@ -2598,7 +2598,8 @@ NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D, const LookupResult ); NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult ); - NamedDecl *getShadowedDeclaration(const BindingDecl *D, const LookupResult ); + NamedDecl *getShadowedDeclaration(const BindingDecl *D, +const LookupResult ); void CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl, const LookupResult ); void CheckShadow(Scope *S, VarDecl *D); Index: clang/include/clang/Sema/Sema.h === --- clang/include/clang/Sema/Sema.h +++ clang/include/clang/Sema/Sema.h @@ -2598,7 +2598,8 @@ NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D, const LookupResult ); NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult ); - NamedDecl *getShadowedDeclaration(const BindingDecl *D, const LookupResult ); + NamedDecl *getShadowedDeclaration(const BindingDecl *D, +const LookupResult ); void CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl, const LookupResult ); void CheckShadow(Scope *S, VarDecl *D); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil added a comment. I have no idea what this error means or what to do about it i'm afraid but it does not look related to the changes I have made. I also do not have access to a Linux machine easily so reproducing it locally is not going to be easy. If anyone knows this error better and can give some pointers that would be great. class/union requires a filename !87 = distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: !2) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96147/new/ https://reviews.llvm.org/D96147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings
Vexthil created this revision.
Vexthil added a reviewer: rsmith.
Vexthil added a project: clang.
Vexthil requested review of this revision.
Herald added a subscriber: cfe-commits.
https://bugs.llvm.org/show_bug.cgi?id=40858
CheckShadow is now called for each binding in the structured binding to make
sure it does not shadow any other variable in scope. This does use a custom
implementation of getShadowedDeclaration though because a BindingDecl is not a
VarDecl
Added a few unit tests for this. In theory though all the other shadow unit
tests should be duplicated for the structured binding variables too but whether
it is probably not worth it as they use common code. The MyTuple and std
interface code has been copied from live-bindings-test.cpp
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D96147
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-shadow.cpp
Index: clang/test/SemaCXX/warn-shadow.cpp
===
--- clang/test/SemaCXX/warn-shadow.cpp
+++ clang/test/SemaCXX/warn-shadow.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
namespace {
int i; // expected-note {{previous declaration is here}}
@@ -265,3 +265,86 @@
PR24718_1 // Does not shadow a type.
};
};
+
+// MyTuple and std code is copied from live-bindings-test.cpp
+
+//#define USE_STD
+
+#ifndef USE_STD
+// Machinery required for custom structured bindings decomposition.
+typedef unsigned long size_t;
+
+namespace std {
+template class tuple_size;
+template
+constexpr size_t tuple_size_v = tuple_size::value;
+template class tuple_element;
+
+template
+struct integral_constant {
+ static constexpr T value = v;
+ typedef T value_type;
+ typedef integral_constant type;
+ constexpr operator value_type() const noexcept { return value; }
+};
+} // namespace std
+
+struct MyTuple {
+ int a;
+ int b;
+
+ template
+ int get() const {
+if constexpr (N == 0)
+ return a;
+else if constexpr (N == 1)
+ return b;
+ }
+};
+
+namespace std {
+template <>
+struct tuple_size
+: std::integral_constant {};
+
+template
+struct tuple_element {
+ using type = int;
+};
+} // namespace std
+
+MyTuple getMyTuple();
+#else
+
+#include
+std::tuple getMyTuple();
+#endif
+
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+
+void test1() {
+ const auto [x, y] = getMyTuple(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+ int a; // expected-note {{previous declaration is here}}
+ bool b; // expected-note {{previous declaration is here}}
+ {
+auto [a, b] = getMyTuple(); // expected-warning 2 {{declaration shadows a local variable}}
+ }
+}
+
+class A
+{
+ int m_a; // expected-note {{previous declaration is here}}
+ int m_b; // expected-note {{previous declaration is here}}
+
+ void test3() {
+auto [m_a, m_b] = getMyTuple(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+ }
+};
+
+}; // namespace structured_binding_tests
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -857,17 +857,28 @@
Previous.clear();
}
+auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+// Find the shadowed declaration before filtering for scope.
+NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+ ? getShadowedDeclaration(BD, Previous)
+ : nullptr;
+
bool ConsiderLinkage = DC->isFunctionOrMethod() &&
DS.getStorageClassSpec() == DeclSpec::SCS_extern;
FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
/*AllowInlineNamespace*/false);
+
+// Diagnose shadowed variables if this isn't a redeclaration.
+if (ShadowedDecl && !D.isRedeclaration())
+ CheckShadow(BD, ShadowedDecl, Previous);
+
if (!Previous.empty()) {
auto *Old = Previous.getRepresentativeDecl();
Diag(B.NameLoc, diag::err_redefinition) << B.Name;
Diag(Old->getLocation(), diag::note_previous_definition);
}
-auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
PushOnScopeChains(BD, S, true);
Bindings.push_back(BD);
ParsingInitForAutoVars.insert(BD);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -7557,6 +7557,19 @@
return isa(ShadowedDecl) ?
