Index: include/clang/Basic/Diagnostic.h =================================================================== --- include/clang/Basic/Diagnostic.h +++ include/clang/Basic/Diagnostic.h @@ -1072,10 +1072,10 @@ // so that we only match those arguments that are (statically) DeclContexts; // other arguments that derive from DeclContext (e.g., RecordDecls) will not // match. -template -inline -typename std::enable_if::value, - const DiagnosticBuilder &>::type +template +inline typename std::enable_if< + std::is_same::type, DeclContext>::value, + const DiagnosticBuilder &>::type operator<<(const DiagnosticBuilder &DB, T *DC) { DB.AddTaggedVal(reinterpret_cast(DC), DiagnosticsEngine::ak_declcontext); Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -352,6 +352,14 @@ "static data member of %2|" "field of %2}1">, InGroup, DefaultIgnore; +def warn_modifying_shadowing_decl : + Warning<"modifying variable %0 that shadows a %select{" + "local variable|" + "variable in %2|" + "static data member of %2|" + "field of %2}1">, + InGroup, DefaultIgnore; + // C++ using declarations def err_using_requires_qualname : Error< Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -1652,6 +1652,17 @@ void DiagnoseFunctionSpecifiers(const DeclSpec &DS); void CheckShadow(Scope *S, VarDecl *D, const LookupResult& R); void CheckShadow(Scope *S, VarDecl *D); + + /// Warn if 'E', which is an expression that is about to be modified, refers + /// to a shadowing declaration. + void CheckShadowingDeclModification(Expr *E, SourceLocation Loc); + +private: + /// 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; + +public: void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange); void handleTagNumbering(const TagDecl *Tag, Scope *TagScope); void setTagNameForLinkagePurposes(TagDecl *TagFromDeclSpec, Index: lib/Sema/SemaDecl.cpp =================================================================== --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -1612,6 +1612,7 @@ // Remove this name from our lexical scope. IdResolver.RemoveDecl(D); + ShadowingDecls.erase(D); } } @@ -6361,6 +6362,23 @@ return NewVD; } +enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field }; + +/// Determine what kind of declaration we're shadowing. The order must be +/// consistent with the %select in the warning message. +static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl, + const DeclContext *OldDC) { + if (isa(OldDC)) { + if (isa(ShadowedDecl)) + return SDK_Field; + else + return SDK_StaticMember; + } else if (OldDC->isFileContext()) { + return SDK_Global; + } + return SDK_Local; +} + /// \brief Diagnose variable or built-in function shadowing. Implements /// -Wshadow. /// @@ -6389,11 +6407,18 @@ if (!isa(ShadowedDecl) && !isa(ShadowedDecl)) return; - // Fields are not shadowed by variables in C++ static methods. - if (isa(ShadowedDecl)) + if (isa(ShadowedDecl)) { + // Fields are not shadowed by variables in C++ static methods. if (CXXMethodDecl *MD = dyn_cast(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(NewDC) && isa(D)) { + ShadowingDecls.insert({D, ShadowedDecl}); + return; + } + } if (VarDecl *shadowedVar = dyn_cast(ShadowedDecl)) if (shadowedVar->isExternC()) { @@ -6422,27 +6447,13 @@ // shadowing context, but that's just a false negative. } - // Determine what kind of declaration we're shadowing. - - // The order must be consistent with the %select in the warning message. - enum ShadowedDeclKind { Local, Global, StaticMember, Field }; - ShadowedDeclKind Kind; - if (isa(OldDC)) { - if (isa(ShadowedDecl)) - Kind = Field; - else - Kind = StaticMember; - } else if (OldDC->isFileContext()) { - Kind = Global; - } else { - Kind = Local; - } DeclarationName Name = R.getLookupName(); // Emit warning and note. if (getSourceManager().isInSystemMacro(R.getNameLoc())) return; + ShadowedDeclKind Kind = computeShadowedDeclKind(ShadowedDecl, OldDC); Diag(R.getNameLoc(), diag::warn_decl_shadow) << Name << Kind << OldDC; Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration); } @@ -6458,6 +6469,30 @@ CheckShadow(S, D, R); } +/// Check if 'E', which is an expression that is about to be modified, refers +/// to a constructor parameter that shadows a field. +void Sema::CheckShadowingDeclModification(Expr *E, SourceLocation Loc) { + // Quickly ignore expressions that can't be shadowing ctor parameters. + if (!getLangOpts().CPlusPlus || ShadowingDecls.empty()) + return; + E = E->IgnoreParenImpCasts(); + auto *DRE = dyn_cast(E); + if (!DRE) + return; + const NamedDecl *D = DRE->getDecl(); + auto I = ShadowingDecls.find(D); + if (I == ShadowingDecls.end()) + return; + const NamedDecl *ShadowedDecl = I->second; + const DeclContext *OldDC = ShadowedDecl->getDeclContext(); + ShadowedDeclKind Kind = computeShadowedDeclKind(ShadowedDecl, OldDC); + Diag(Loc, diag::warn_modifying_shadowing_decl) << D << Kind << OldDC; + Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration); + + // Avoid issuing multiple warnings about the same decl. + ShadowingDecls.erase(I); +} + /// 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 @@ -9658,6 +9658,9 @@ /// emit an error and return true. If so, return false. static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) { assert(!E->hasPlaceholderType(BuiltinType::PseudoObject)); + + S.CheckShadowingDeclModification(E, Loc); + SourceLocation OrigLoc = Loc; Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context, &Loc); Index: test/SemaCXX/warn-shadow.cpp =================================================================== --- test/SemaCXX/warn-shadow.cpp +++ test/SemaCXX/warn-shadow.cpp @@ -29,7 +29,18 @@ class A { static int data; // expected-note {{previous declaration}} - int field; // expected-note {{previous declaration}} + // expected-note@+1 {{previous declaration}} + int field; + int f1, f2, f3, f4; // expected-note 4 {{previous declaration is here}} + + // The initialization is safe, but the modifications are not. + A(int f1, int f2, int f3, int f4) : f1(f1) { + f1 = 3; // expected-warning {{modifying variable 'f1' that shadows a field of 'A'}} + f1 = 4; // one warning per shadow + f2++; // expected-warning {{modifying variable 'f2' that shadows a field of 'A'}} + --f3; // expected-warning {{modifying variable 'f3' that shadows a field of 'A'}} + f4 += 2; // expected-warning {{modifying variable 'f4' that shadows a field of 'A'}} + } void test() { char *field; // expected-warning {{declaration shadows a field of 'A'}}