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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
