arphaman updated this revision to Diff 78005.
arphaman added a comment.
Fixed an existing comment that now became partially invalid.
Repository:
rL LLVM
https://reviews.llvm.org/D26672
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExpr.cpp
test/SemaCXX/warn-shadow-in-setter-methods.cpp
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<Foo*>(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;
+ }
+};
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: 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<FieldDecl>(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<FieldDecl>(ShadowI->second.ShadowedDecl)) {
+ if (isa<CXXConstructorDecl>(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);
}
@@ -6627,13 +6630,14 @@
if (MD->isStatic())
return;
- // Fields shadowed by constructor parameters are a special case. Usually
- // the constructor initializes the field with the parameter.
- if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) {
+ // Fields shadowed by method parameters are a special case. Constructors
+ // usually initialize the field with the parameter, and certain methods,
+ // like setters, usually just use the parameter to assign it to the field.
+ if (isa<CXXMethodDecl>(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});
+ ShadowingDecls.insert({D, {FD, 0}});
return;
}
}
@@ -6745,9 +6749,10 @@
return;
const NamedDecl *D = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl());
auto I = ShadowingDecls.find(D);
- if (I == ShadowingDecls.end())
+ if (I == ShadowingDecls.end() ||
+ !isa<CXXConstructorDecl>(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 +6762,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 method parameters.
+ if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
+ return;
+ const NamedDecl *D = cast<NamedDecl>(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 method parameters.
+ if (!getLangOpts().CPlusPlus || ShadowingDecls.empty())
+ return;
+ const auto *DRE = dyn_cast<DeclRefExpr>(RHS->IgnoreParenCasts());
+ if (!DRE || !isa<ParmVarDecl>(DRE->getDecl()))
+ return;
+ const auto *ME = dyn_cast<MemberExpr>(LHS->IgnoreParenImpCasts());
+ if (!ME || !isa<CXXThisExpr>(ME->getBase()->IgnoreParenCasts()))
+ return;
+ const NamedDecl *D = cast<NamedDecl>(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<typename T>
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<const NamedDecl *, const NamedDecl *> ShadowingDecls;
+ llvm::DenseMap<const NamedDecl *, ShadowedDeclInfo> ShadowingDecls;
public:
void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange);
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<ShadowFieldInConstructorModified>, DefaultIgnore;
+def warn_setter_parm_shadows_field :
+ Warning<warn_decl_shadow.Text>,
+ InGroup<ShadowFieldInSetter>, DefaultIgnore;
// C++ decomposition declarations
def err_decomp_decl_context : Error<
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">;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
