rnk updated this revision to Diff 52983. rnk added a comment. - Add a test for -Wshadow-all
http://reviews.llvm.org/D18271 Files: include/clang/Basic/Diagnostic.h include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/AnalysisBasedWarnings.cpp 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 @@ -1,4 +1,5 @@ // RUN: %clang_cc1 -verify -fsyntax-only -Wshadow %s +// RUN: %clang_cc1 -verify -fsyntax-only -Wshadow-all %s -DSHADOW_ALL namespace { int i; // expected-note {{previous declaration is here}} @@ -29,7 +30,25 @@ 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}} + +#ifdef SHADOW_ALL + // This mode is very strict, we just warn. + // 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) {} +#else + // 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 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'}} + } +#endif 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 @@ -9671,6 +9671,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: 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); } } @@ -6363,6 +6364,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<RecordDecl>(OldDC)) + return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember; + return OldDC->isFileContext() ? SDK_Global : SDK_Local; +} + /// \brief Diagnose variable or built-in function shadowing. Implements /// -Wshadow. /// @@ -6391,12 +6403,29 @@ if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl)) return; - // Fields are not shadowed by variables in C++ static methods. - if (isa<FieldDecl>(ShadowedDecl)) + if (FieldDecl *FD = dyn_cast<FieldDecl>(ShadowedDecl)) { + // Fields are not shadowed by variables in C++ static methods. if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC)) if (MD->isStatic()) return; + // Fields shadowed by constructor parameters are a special case. Usually + // the constructor initializes the field with the parameter. If the user + // wants us to be strict, warn immediately. Otherwise, store this decl in + // our side map and see if it gets modified later. + if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) { + if (Diags.isIgnored(diag::warn_ctor_parm_shadows_field, R.getNameLoc())) { + D = D->getCanonicalDecl(); + ShadowingDecls.insert({D, FD}); + } else { + Diag(R.getNameLoc(), diag::warn_ctor_parm_shadows_field) + << D << FD << FD->getParent(); + Diag(FD->getLocation(), diag::note_previous_declaration); + } + return; + } + } + if (VarDecl *shadowedVar = dyn_cast<VarDecl>(ShadowedDecl)) if (shadowedVar->isExternC()) { // For shadowing external vars, make sure that we point to the global @@ -6424,27 +6453,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<RecordDecl>(OldDC)) { - if (isa<FieldDecl>(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); } @@ -6460,6 +6475,31 @@ 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<DeclRefExpr>(E); + if (!DRE) + return; + const NamedDecl *D = cast<NamedDecl>(DRE->getDecl()->getCanonicalDecl()); + 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(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<typename T> Index: lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- lib/Sema/AnalysisBasedWarnings.cpp +++ 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: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ 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<const NamedDecl *, const NamedDecl *> ShadowingDecls; + +public: void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange); void handleTagNumbering(const TagDecl *Tag, Scope *TagScope); void setTagNameForLinkagePurposes(TagDecl *TagFromDeclSpec, Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -352,6 +352,17 @@ "static data member of %2|" "field of %2}1">, InGroup<Shadow>, DefaultIgnore; +def warn_ctor_parm_shadows_field: + Warning<"constructor parameter %0 shadows the field %1 of %2">, + InGroup<ShadowFieldInConstructor>, 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<Shadow>, DefaultIgnore; + // C++ using declarations def err_using_requires_qualname : Error< @@ -1667,7 +1678,7 @@ "variable %0 may be uninitialized when " "%select{used here|captured by block}1">, InGroup<UninitializedMaybe>, 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: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -331,7 +331,13 @@ def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">; def Sentinel : DiagGroup<"sentinel">; def MissingMethodReturnType : DiagGroup<"missing-method-return-type">; + +// -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the +// shadowing that we think is unsafe. def Shadow : DiagGroup<"shadow">; +def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor">; +def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor]>; + def Shorten64To32 : DiagGroup<"shorten-64-to-32">; def : DiagGroup<"sign-promo">; def SignCompare : DiagGroup<"sign-compare">; 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<typename T> -inline -typename std::enable_if<std::is_same<T, DeclContext>::value, - const DiagnosticBuilder &>::type +template <typename T> +inline typename std::enable_if< + std::is_same<typename std::remove_const<T>::type, DeclContext>::value, + const DiagnosticBuilder &>::type operator<<(const DiagnosticBuilder &DB, T *DC) { DB.AddTaggedVal(reinterpret_cast<intptr_t>(DC), DiagnosticsEngine::ak_declcontext);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits