[PATCH] D18271: Avoid -Wshadow warnings about constructor parameters named after fields

[Richard Smith via cfe-commits]

rsmith added a comment.

I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter, and all uses of the parameter in the constructor body would still be valid if it were declared `const`".

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain `field(field)`.

(I agree that catching `A(X x) : x(std::move(x)) { use(x); }` is outside the scope of this change.)


================
Comment at: lib/Sema/SemaDecl.cpp:6400
@@ +6399,3 @@
+    if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D))
+      return;
+  }
----------------
Instead of redoing class member lookup every time we check to see if a variable is modifiable, perhaps you could populate a set of shadowed-but-not-diagnosed `VarDecl*`s here? That way, you can also suppress the duplicate diagnostics if the variable is modified multiple times by removing it from the set.

================
Comment at: lib/Sema/SemaExpr.cpp:9663
@@ +9662,3 @@
+  if (!S.getLangOpts().CPlusPlus ||
+      S.Diags.isIgnored(diag::warn_decl_shadow, Loc))
+    return;
----------------
This query is actually quite expensive (more so than some or all of the other checks below).


http://reviews.llvm.org/D18271