[PATCH] D31235: Enhance -Wshadow to warn when shadowing typedefs or type aliases

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:5547
 
+  if (ShadowedDecl && !Redeclaration) {
+    CheckShadow(NewTD, ShadowedDecl, Previous);
----------------
You don't need to use `{}` braces here.


================
Comment at: lib/Sema/SemaDecl.cpp:6753
     // the constructor initializes the field with the parameter.
-    if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) {
-      // Remember that this was shadowed so we can either warn about its
-      // modification or its existence depending on warning settings.
-      D = D->getCanonicalDecl();
-      ShadowingDecls.insert({D, FD});
-      return;
-    }
+    if (isa<CXXConstructorDecl>(NewDC))
+      if (ParmVarDecl* PVD = dyn_cast<ParmVarDecl>(D)) {
----------------
ahmedasadi wrote:
> arphaman wrote:
> > Why is the change to this `if` necessary? It doesn't seem that related to the main change.
> VarDecl overrides getCanonicalDecl() to return a VarDecl*. As the type of D was changed from VarDecl* to NamedDecl*,  getCanonicalDecl() now returns a NamedDecl*. 
> 
> I created a new ParmVarDecl variable so getCanonicalDecl() returns a VarDecl* which can be inserted into ShadowingDecls.
> 
> Perhaps it might be better to just cast D->getCanonicalDecl() to a VarDecl when inserting it into ShadowingDecls?
I see, thanks for the explanation. The change is fine then.


================
Comment at: lib/Sema/SemaDecl.cpp:6754
+    if (isa<CXXConstructorDecl>(NewDC))
+      if (ParmVarDecl* PVD = dyn_cast<ParmVarDecl>(D)) {
+        // Remember that this was shadowed so we can either warn about its
----------------
You can use `const auto` here.


================
Comment at: test/SemaCXX/warn-shadow.cpp:65
     char *data; // expected-warning {{declaration shadows a static data member of 'A'}}
+    char *a1; // expected-warning {{declaration shadows a typedef in 'A'}}
+    char *a2; // expected-warning {{declaration shadows a type alias in 'A'}}
----------------
It looks like previously this wouldn't have been a warning. Should we really warn about local variables that shadow typedef names?


https://reviews.llvm.org/D31235