Index: cfe/trunk/include/clang/Basic/Diagnostic.h =================================================================== --- cfe/trunk/include/clang/Basic/Diagnostic.h +++ cfe/trunk/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: cfe/trunk/include/clang/Basic/DiagnosticGroups.td =================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td @@ -337,7 +337,16 @@ def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">; def Sentinel : DiagGroup<"sentinel">; def MissingMethodReturnType : DiagGroup<"missing-method-return-type">; -def Shadow : DiagGroup<"shadow">; + +def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">; +def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor", + [ShadowFieldInConstructorModified]>; + +// -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the +// shadowing that we think is unsafe. +def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified]>; +def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor]>; + def Shorten64To32 : DiagGroup<"shorten-64-to-32">; def : DiagGroup<"sign-promo">; def SignCompare : DiagGroup<"sign-compare">; Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -352,6 +352,14 @@ "static data member of %2|" "field of %2}1">, InGroup, DefaultIgnore; +def warn_ctor_parm_shadows_field: + Warning<"constructor parameter %0 shadows the field %1 of %2">, + InGroup, DefaultIgnore; +def warn_modifying_shadowing_decl : + Warning<"modifying constructor parameter %0 that shadows a " + "field of %1">, + InGroup, DefaultIgnore; + // C++ using declarations def err_using_requires_qualname : Error< @@ -1667,7 +1675,7 @@ "variable %0 may be uninitialized when " "%select{used here|captured by block}1">, InGroup, DefaultIgnore; -def note_uninit_var_def : Note<"variable %0 is declared here">; +def note_var_declared_here : Note<"variable %0 is declared here">; def note_uninit_var_use : Note< "%select{uninitialized use occurs|variable is captured by block}0 here">; def warn_uninit_byref_blockvar_captured_by_block : Warning< Index: cfe/trunk/include/clang/Sema/Sema.h =================================================================== --- cfe/trunk/include/clang/Sema/Sema.h +++ cfe/trunk/include/clang/Sema/Sema.h @@ -1653,6 +1653,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: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp @@ -889,7 +889,7 @@ // the initializer of that declaration & we didn't already suggest // an initialization fixit. if (!SuggestInitializationFixit(S, VD)) - S.Diag(VD->getLocStart(), diag::note_uninit_var_def) + S.Diag(VD->getLocStart(), diag::note_var_declared_here) << VD->getDeclName(); return true; Index: cfe/trunk/lib/Sema/SemaDecl.cpp =================================================================== --- cfe/trunk/lib/Sema/SemaDecl.cpp +++ cfe/trunk/lib/Sema/SemaDecl.cpp @@ -1609,9 +1609,19 @@ // If this was a forward reference to a label, verify it was defined. if (LabelDecl *LD = dyn_cast(D)) CheckPoppedLabel(LD, *this); - - // Remove this name from our lexical scope. + + // Remove this name from our lexical scope, and warn on it if we haven't + // already. 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); + } + ShadowingDecls.erase(ShadowI); + } } } @@ -6382,6 +6392,17 @@ return NewVD; } +/// Enum describing the %select options in diag::warn_decl_shadow. +enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field }; + +/// Determine what kind of declaration we're shadowing. +static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl, + const DeclContext *OldDC) { + if (isa(OldDC)) + return isa(ShadowedDecl) ? SDK_Field : SDK_StaticMember; + return OldDC->isFileContext() ? SDK_Global : SDK_Local; +} + /// \brief Diagnose variable or built-in function shadowing. Implements /// -Wshadow. /// @@ -6410,12 +6431,23 @@ if (!isa(ShadowedDecl) && !isa(ShadowedDecl)) return; - // Fields are not shadowed by variables in C++ static methods. - if (isa(ShadowedDecl)) + if (FieldDecl *FD = dyn_cast(ShadowedDecl)) { + // Fields are not shadowed by variables in C++ static methods. if (CXXMethodDecl *MD = dyn_cast(NewDC)) if (MD->isStatic()) return; + // Fields shadowed by constructor parameters are a special case. Usually + // the constructor initializes the field with the parameter. + 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}); + return; + } + } + if (VarDecl *shadowedVar = dyn_cast(ShadowedDecl)) if (shadowedVar->isExternC()) { // For shadowing external vars, make sure that we point to the global @@ -6443,27 +6475,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); } @@ -6479,6 +6497,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 = cast(DRE->getDecl()->getCanonicalDecl()); + auto I = ShadowingDecls.find(D); + if (I == ShadowingDecls.end()) + return; + const NamedDecl *ShadowedDecl = I->second; + const DeclContext *OldDC = ShadowedDecl->getDeclContext(); + Diag(Loc, diag::warn_modifying_shadowing_decl) << D << OldDC; + Diag(D->getLocation(), diag::note_var_declared_here) << D; + 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: cfe/trunk/lib/Sema/SemaExpr.cpp =================================================================== --- cfe/trunk/lib/Sema/SemaExpr.cpp +++ cfe/trunk/lib/Sema/SemaExpr.cpp @@ -9733,6 +9733,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: cfe/trunk/test/SemaCXX/warn-shadow.cpp =================================================================== --- cfe/trunk/test/SemaCXX/warn-shadow.cpp +++ cfe/trunk/test/SemaCXX/warn-shadow.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow %s +// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow-all %s namespace { int i; // expected-note {{previous declaration is here}} @@ -29,7 +29,23 @@ 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 8 {{previous declaration is here}} + + // The initialization is safe, but the modifications are not. + A(int f1, int f2, int f3, int f4) // expected-note-re 4 {{variable 'f{{[0-4]}}' is declared here}} + : f1(f1) { + f1 = 3; // expected-warning {{modifying constructor parameter 'f1' that shadows a field of 'A'}} + f1 = 4; // one warning per shadow + f2++; // expected-warning {{modifying constructor parameter 'f2' that shadows a field of 'A'}} + --f3; // expected-warning {{modifying constructor parameter 'f3' that shadows a field of 'A'}} + f4 += 2; // expected-warning {{modifying constructor parameter 'f4' that shadows a field of 'A'}} + } + + // The initialization is safe, but the modifications are not. + // expected-warning-re@+1 4 {{constructor parameter 'f{{[0-4]}}' shadows the field 'f{{[0-9]}}' of 'A'}} + A(int f1, int f2, int f3, int f4, double overload_dummy) {} void test() { char *field; // expected-warning {{declaration shadows a field of 'A'}}