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

[Reid Kleckner via cfe-commits]

rnk added a comment.

In http://reviews.llvm.org/D18271#381769, @rsmith wrote:

> 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`".


That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?

> 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)`.


Would the reference binding check be somewhere in SemaInit.cpp? I don't see an obvious spot to do this check at first glance.

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


Wouldn't the proposed check for non-const references find this example, though?


================
Comment at: lib/Sema/SemaDecl.cpp:6400
@@ +6399,3 @@
+    if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D))
+      return;
+  }
----------------
rsmith wrote:
> 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.
That sounds like a good idea. I could even leave it empty if we're not doing -Wshadow.

================
Comment at: lib/Sema/SemaExpr.cpp:9663
@@ +9662,3 @@
+  if (!S.getLangOpts().CPlusPlus ||
+      S.Diags.isIgnored(diag::warn_decl_shadow, Loc))
+    return;
----------------
rsmith wrote:
> This query is actually quite expensive (more so than some or all of the other checks below).
This check has to be faster than name lookup, right? Maybe do it before that?


http://reviews.llvm.org/D18271