rnk created this revision.
rnk added reviewers: rsmith, rtrieu.
rnk added a subscriber: cfe-commits.
Emit a new warning if such parameters are modified in the body of the
constructor.
Fixes PR16088.
http://reviews.llvm.org/D18271
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
lib/Sema/SemaExpr.cpp
test/SemaCXX/warn-shadow.cpp
Index: test/SemaCXX/warn-shadow.cpp
===================================================================
--- test/SemaCXX/warn-shadow.cpp
+++ test/SemaCXX/warn-shadow.cpp
@@ -29,7 +29,16 @@
class A {
static int data; // expected-note {{previous declaration}}
- int field; // expected-note {{previous declaration}}
+ // expected-note@+1 {{previous declaration}}
+ int field; // expected-note 4 {{member 'field' is declared here}}
+
+ // The initialization is safe, but the modifications are not.
+ A(int field) : field(field) {
+ field = 3; // expected-warning {{modifying constructor parameter 'field' that shadows field of 'A'}}
+ field++; // expected-warning {{modifying constructor parameter 'field' that shadows field of 'A'}}
+ --field; // expected-warning {{modifying constructor parameter 'field' that shadows field of 'A'}}
+ field += 2; // expected-warning {{modifying constructor parameter 'field' that shadows field of 'A'}}
+ }
void test() {
char *field; // expected-warning {{declaration shadows a field of 'A'}}
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9654,10 +9654,42 @@
S.Diag(Loc, diag::err_typecheck_assign_const) << ExprRange << ConstUnknown;
}
+/// Check if 'E', which is an expression that is about to be modified, refers
+/// to a constructor parameter which shadows a field.
+static void checkForShadowedCtorParameter(Sema &S, Expr *E, SourceLocation Loc) {
+ // Return early before doing lookup if the warning can't happen or would be
+ // ignored.
+ if (!S.getLangOpts().CPlusPlus ||
+ S.Diags.isIgnored(diag::warn_decl_shadow, Loc))
+ return;
+ auto *DRE = dyn_cast<DeclRefExpr>(E);
+ const NamedDecl *D = DRE->getDecl();
+ if (!isa<ParmVarDecl>(D))
+ return;
+ const DeclContext *DC = D->getDeclContext();
+ auto *CD = dyn_cast<CXXConstructorDecl>(DC);
+ if (!CD)
+ return;
+ const CXXRecordDecl *Record = CD->getParent();
+ DeclContextLookupResult Lookup = Record->lookup(D->getDeclName());
+ const NamedDecl *ShadowedDecl =
+ Lookup.size() == 1 ? Lookup.front() : nullptr;
+ // If there is no shadowing or the shadowed decl is not a field, we don't
+ // care. It has already been diagnosed.
+ if (!ShadowedDecl || !isa<FieldDecl>(ShadowedDecl))
+ return;
+ S.Diag(Loc, diag::warn_modifying_shadowed_field) << D << Record;
+ S.Diag(ShadowedDecl->getLocation(), diag::note_member_declared_here)
+ << ShadowedDecl;
+}
+
/// CheckForModifiableLvalue - Verify that E is a modifiable lvalue. If not,
/// emit an error and return true. If so, return false.
static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
assert(!E->hasPlaceholderType(BuiltinType::PseudoObject));
+
+ checkForShadowedCtorParameter(S, E, Loc);
+
SourceLocation OrigLoc = Loc;
Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context,
&Loc);
@@ -9852,12 +9884,12 @@
}
if (ConvTy == Compatible) {
+ const Expr *InnerLHS = LHSExpr->IgnoreParenCasts();
+ const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InnerLHS);
if (LHSType.getObjCLifetime() == Qualifiers::OCL_Strong) {
// Warn about retain cycles where a block captures the LHS, but
// not if the LHS is a simple variable into which the block is
// being stored...unless that variable can be captured by reference!
- const Expr *InnerLHS = LHSExpr->IgnoreParenCasts();
- const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InnerLHS);
if (!DRE || DRE->getDecl()->hasAttr<BlocksAttr>())
checkRetainCycles(LHSExpr, RHS.get());
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -6389,11 +6389,16 @@
if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl))
return;
- // Fields are not shadowed by variables in C++ static methods.
- if (isa<FieldDecl>(ShadowedDecl))
+ if (isa<FieldDecl>(ShadowedDecl)) {
+ // Fields are not shadowed by variables in C++ static methods.
if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC))
if (MD->isStatic())
return;
+ // Fields are not shadowed by constructor parameters. This allows a popular
+ // pattern where constructor parameters are named after fields.
+ if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D))
+ return;
+ }
if (VarDecl *shadowedVar = dyn_cast<VarDecl>(ShadowedDecl))
if (shadowedVar->isExternC()) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -369,6 +369,10 @@
"static data member of %2|"
"field of %2}1">,
InGroup<Shadow>, DefaultIgnore;
+def warn_modifying_shadowed_field :
+ Warning<"modifying constructor parameter %0 that shadows field of %1">,
+ InGroup<Shadow>, DefaultIgnore;
+
// C++ using declarations
def err_using_requires_qualname : Error<
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
