Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -352,13 +352,15 @@ [ShadowFieldInConstructorModified]>; def ShadowIvar : DiagGroup<"shadow-ivar">; def ShadowUncapturedLocal : DiagGroup<"shadow-uncaptured-local">; +def ShadowFieldInSetter : DiagGroup<"shadow-field-in-setter">; // -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the // shadowing that we think is unsafe. def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified, ShadowIvar]>; def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor, - ShadowUncapturedLocal]>; + ShadowUncapturedLocal, + ShadowFieldInSetter]>; def Shorten64To32 : DiagGroup<"shorten-64-to-32">; def : DiagGroup<"sign-promo">; Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -367,6 +367,9 @@ Warning<"modifying constructor parameter %0 that shadows a " "field of %1">, InGroup, DefaultIgnore; +def warn_setter_parm_shadows_field : + Warning, + InGroup, DefaultIgnore; // C++ decomposition declarations def err_decomp_decl_context : Error< Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -1718,12 +1718,27 @@ /// to a shadowing declaration. void CheckShadowingDeclModification(Expr *E, SourceLocation Loc); + void CheckShadowingDeclUse(const DeclRefExpr *E); + void CheckShadowingAssignmentOperands(const Expr *LHS, const Expr *RHS); + void DiagnoseShadowingLambdaDecls(const sema::LambdaScopeInfo *LSI); + void DiagnoseMethodParamShadowingField(const NamedDecl *D, + const FieldDecl *ShadowedDecl, + unsigned UsageCount); private: + /// Contains the shadowed declaration along with additional info like how + /// many times was the shadowing declaration used. + struct ShadowedDeclInfo { + const NamedDecl *ShadowedDecl; + /// How many times was the declaration used non-idiomatically, where it can + /// be confused for the shadowed declaration in the source code. + unsigned UsageCount; + }; + /// Map of current shadowing declarations to shadowed declarations. Warn if /// it looks like the user is trying to modify the shadowing declaration. - llvm::DenseMap ShadowingDecls; + llvm::DenseMap ShadowingDecls; public: void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange); Index: lib/Sema/SemaDecl.cpp =================================================================== --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -1665,10 +1665,13 @@ IdResolver.RemoveDecl(D); auto ShadowI = ShadowingDecls.find(D); if (ShadowI != ShadowingDecls.end()) { - if (const auto *FD = dyn_cast(ShadowI->second)) { - Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field) - << D << FD << FD->getParent(); - Diag(FD->getLocation(), diag::note_previous_declaration); + if (const auto *FD = dyn_cast(ShadowI->second.ShadowedDecl)) { + if (isa(D->getDeclContext())) { + Diag(D->getLocation(), diag::warn_ctor_parm_shadows_field) + << D << FD << FD->getParent(); + Diag(FD->getLocation(), diag::note_previous_declaration); + } else + DiagnoseMethodParamShadowingField(D, FD, ShadowI->second.UsageCount); } ShadowingDecls.erase(ShadowI); } @@ -6629,11 +6632,11 @@ // Fields shadowed by constructor parameters are a special case. Usually // the constructor initializes the field with the parameter. - if (isa(NewDC) && isa(D)) { + if (isa(NewDC) && isa(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}); + ShadowingDecls.insert({D, {FD, 0}}); return; } } @@ -6745,9 +6748,10 @@ return; const NamedDecl *D = cast(DRE->getDecl()->getCanonicalDecl()); auto I = ShadowingDecls.find(D); - if (I == ShadowingDecls.end()) + if (I == ShadowingDecls.end() || + !isa(D->getDeclContext())) return; - const NamedDecl *ShadowedDecl = I->second; + const NamedDecl *ShadowedDecl = I->second.ShadowedDecl; const DeclContext *OldDC = ShadowedDecl->getDeclContext(); Diag(Loc, diag::warn_modifying_shadowing_decl) << D << OldDC; Diag(D->getLocation(), diag::note_var_declared_here) << D; @@ -6757,6 +6761,59 @@ ShadowingDecls.erase(I); } +/// Check if \p E refers to a method parameter that shadows a field. +void Sema::CheckShadowingDeclUse(const DeclRefExpr *E) { + // Quickly ignore expressions that can't be shadowing ctor parameters. + if (!getLangOpts().CPlusPlus || ShadowingDecls.empty()) + return; + const NamedDecl *D = cast(E->getDecl()->getCanonicalDecl()); + auto I = ShadowingDecls.find(D); + if (I == ShadowingDecls.end()) + return; + I->second.UsageCount++; +} + +/// Check if assignment expression like \p LHS = \p RHS contains an idiomatic +/// usage of a method parameter that shadows a field. +/// +/// This is used to avoid -Wshadow warnings for setter-like methods, e.g.: +/// +/// struct Foo { int x; +/// void setX(int x) { this->x = x; } // Avoid -Wshadow here. +/// }; +void Sema::CheckShadowingAssignmentOperands(const Expr *LHS, const Expr *RHS) { + // Quickly ignore expressions that can't be shadowing ctor parameters. + if (!getLangOpts().CPlusPlus || ShadowingDecls.empty()) + return; + const auto *DRE = dyn_cast(RHS->IgnoreParenCasts()); + if (!DRE || !isa(DRE->getDecl())) + return; + const auto *ME = dyn_cast(LHS->IgnoreParenImpCasts()); + if (!ME || !isa(ME->getBase()->IgnoreParenCasts())) + return; + const NamedDecl *D = cast(DRE->getDecl()->getCanonicalDecl()); + auto I = ShadowingDecls.find(D); + if (I == ShadowingDecls.end()) + return; + const NamedDecl *ShadowedDecl = I->second.ShadowedDecl; + if (ShadowedDecl != ME->getMemberDecl()) + return; + I->second.UsageCount--; +} + +/// Diagnose shadowing for method parameters that shadow fields. +void Sema::DiagnoseMethodParamShadowingField(const NamedDecl *D, + const FieldDecl *ShadowedDecl, + unsigned UsageCount) { + const DeclContext *OldDC = ShadowedDecl->getDeclContext(); + Diag(D->getLocation(), UsageCount || !D->isUsed() + ? diag::warn_decl_shadow + : diag::warn_setter_parm_shadows_field) + << D->getDeclName() << computeShadowedDeclKind(ShadowedDecl, OldDC) + << OldDC; + Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration); +} + /// Check for conflict between this global or extern "C" declaration and /// previous global or extern "C" declarations. This is only used in C++. template Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -10136,6 +10136,9 @@ if (CheckForModifiableLvalue(LHSExpr, Loc, *this)) return QualType(); + if (RHS.isUsable()) + CheckShadowingAssignmentOperands(LHSExpr, RHS.get()); + QualType LHSType = LHSExpr->getType(); QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() : CompoundType; @@ -14230,6 +14233,7 @@ if (Method->isVirtual()) OdrUse = false; MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse); + CheckShadowingDeclUse(E); } /// \brief Perform reference-marking and odr-use handling for a MemberExpr. Index: test/SemaCXX/warn-shadow-in-setter-methods.cpp =================================================================== --- /dev/null +++ test/SemaCXX/warn-shadow-in-setter-methods.cpp @@ -0,0 +1,41 @@ +// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -D AVOID %s +// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -Wshadow-field-in-setter %s +// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow-all %s + +struct Foo { + int x; // expected-note 1+ {{previous declaration is here}} + int y; // expected-note 1+ {{previous declaration is here}} + + // The following setter-like methods should avoid -Wshadow for parameters 'x' and 'y': + + void setter1(int x) { this->x = x; } // warn here only + void setter2(int x) const { const_cast(this)->x = x; } + void setter3(int x) { this->x *= (int)((x)); } + void setter4(int x) { this->x += x; } + void setter5(int x) { (this->x) >>= x; } + void setter6(int x) { this->x = x; this->x -= x; } + void setter7(int x, int y) { this->x = x; this->y = y; } +#ifndef AVOID + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-8 {{declaration shadows a field of 'Foo'}} + // expected-warning@-9 {{declaration shadows a field of 'Foo'}} +#endif + + // The following methods should still warn for parameters 'x' and 'y':: + + void warn1(int x) { this->x = x + x; } // expected-warning {{declaration shadows a field of 'Foo'}} + void warn2(int x) { setter1(x); } // expected-warning {{declaration shadows a field of 'Foo'}} + void warn3(int x) { } // expected-warning {{declaration shadows a field of 'Foo'}} + void warn4(int x, int y) { // expected-warning {{declaration shadows a field of 'Foo'}} + this->x = y; // expected-warning@-1 {{declaration shadows a field of 'Foo'}} + this->y = x; + } + void warn5(int y) { // expected-warning {{declaration shadows a field of 'Foo'}} + (void)y; + } +};