From f463a8a424e7e1c141aa7d9b96f1898183d2b680 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Fri, 29 Apr 2016 00:37:43 +0000 Subject: [PATCH] Avoid -Wshadow warnings about constructor parameters named after fields Usually these parameters are used solely to initialize the field in the initializer list, and there is no real shadowing confusion. There is a new warning under -Wshadow called -Wshadow-field-in-constructor-modified. It attempts to find modifications of such constructor parameters that probably intended to modify the field. It has some false negatives, though, so there is another warning group, -Wshadow-field-in-constructor, which always warns on this special case. For users who just want the old behavior and don't care about these fine grained groups, we have a new warning group called -Wshadow-all that activates everything. Fixes PR16088. Reviewers: rsmith Subscribers: cfe-commits Differential Revision: http://reviews.llvm.org/D18271 llvm-svn: 267957 --- clang/include/clang/Basic/Diagnostic.h | 8 +- clang/include/clang/Basic/DiagnosticGroups.td | 11 ++- .../clang/Basic/DiagnosticSemaKinds.td | 10 ++- clang/include/clang/Sema/Sema.h | 11 +++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 2 +- clang/lib/Sema/SemaDecl.cpp | 80 ++++++++++++++----- clang/lib/Sema/SemaExpr.cpp | 3 + clang/test/SemaCXX/warn-shadow.cpp | 20 ++++- 8 files changed, 117 insertions(+), 28 deletions(-) diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index dd4443e5dd1f..085faeae4834 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -1072,10 +1072,10 @@ inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB, // 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); diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 8b5d8df6fc4f..7b4740fe22a2 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -337,7 +337,16 @@ def SelfMove : DiagGroup<"self-move">; 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">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index b04abc55afbb..cd7a7557d073 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -352,6 +352,14 @@ def warn_decl_shadow : "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 @@ def warn_maybe_uninit_var : Warning< "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< diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index f53bc05cc1e7..14f9d2c86fc9 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1653,6 +1653,17 @@ public: 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, diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index fcb12f6cebf9..3f2c41b67bcc 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -889,7 +889,7 @@ static bool DiagnoseUninitializedUse(Sema &S, const VarDecl *VD, // 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; diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 03f235d4399c..0c8781c02338 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -1609,9 +1609,19 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { // 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 @@ Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC, 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 @@ void Sema::CheckShadow(Scope *S, VarDecl *D, const LookupResult& R) { 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 @@ void Sema::CheckShadow(Scope *S, VarDecl *D, const LookupResult& R) { // 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 @@ void Sema::CheckShadow(Scope *S, VarDecl *D) { 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 diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 065c236d6cc5..6b043368db1b 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -9733,6 +9733,9 @@ static void DiagnoseConstAssignment(Sema &S, const Expr *E, /// 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); diff --git a/clang/test/SemaCXX/warn-shadow.cpp b/clang/test/SemaCXX/warn-shadow.cpp index 5ad2233d234a..9d68fe7c47a6 100644 --- a/clang/test/SemaCXX/warn-shadow.cpp +++ b/clang/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 @@ void foo() { 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'}}