rsmith added a comment.
Thanks!
================
Comment at: clang/lib/Sema/SemaDecl.cpp:7571
+ NamedDecl *ShadowedDecl = R.getFoundDecl();
+ return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)
+ ? ShadowedDecl
----------------
rsmith wrote:
> I think we should also warn if a `BindingDecl` shadows another `BindingDecl`,
> or if a `VarDecl` shadows a `BindingDecl`.
`isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)` can be simplified
to `isa<VarDecl, FieldDecl>(ShadowedDecl)`.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:7571-7573
+ return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(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
