[PATCH] D96147: [SEMA] Added warn_decl_shadow support for structured bindings

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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