r267957 - Avoid -Wshadow warnings about constructor parameters named after fields
Reid Kleckner via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 28 17:37:44 PDT 2016
Author: rnk
Date: Thu Apr 28 19:37:43 2016
New Revision: 267957
URL: http://llvm.org/viewvc/llvm-project?rev=267957&view=rev
Log:
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
Modified:
cfe/trunk/include/clang/Basic/Diagnostic.h
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/SemaCXX/warn-shadow.cpp
Modified: cfe/trunk/include/clang/Basic/Diagnostic.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/Diagnostic.h (original)
+++ cfe/trunk/include/clang/Basic/Diagnostic.h Thu Apr 28 19:37:43 2016
@@ -1072,10 +1072,10 @@ inline const DiagnosticBuilder &operator
// 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);
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Apr 28 19:37:43 2016
@@ -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">;
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Apr 28 19:37:43 2016
@@ -352,6 +352,14 @@ def warn_decl_shadow :
"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 constructor parameter %0 that shadows a "
+ "field of %1">,
+ InGroup<ShadowFieldInConstructorModified>, 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<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<
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Apr 28 19:37:43 2016
@@ -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<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,
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Thu Apr 28 19:37:43 2016
@@ -889,7 +889,7 @@ static bool DiagnoseUninitializedUse(Sem
// 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;
Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Apr 28 19:37:43 2016
@@ -1609,9 +1609,19 @@ void Sema::ActOnPopScope(SourceLocation
// If this was a forward reference to a label, verify it was defined.
if (LabelDecl *LD = dyn_cast<LabelDecl>(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<FieldDecl>(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,
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.
///
@@ -6410,12 +6431,23 @@ void Sema::CheckShadow(Scope *S, VarDecl
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 (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(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<VarDecl>(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
// 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);
}
@@ -6479,6 +6497,30 @@ void Sema::CheckShadow(Scope *S, VarDecl
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();
+ 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<typename T>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Apr 28 19:37:43 2016
@@ -9733,6 +9733,9 @@ static void DiagnoseConstAssignment(Sema
/// 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);
Modified: cfe/trunk/test/SemaCXX/warn-shadow.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-shadow.cpp?rev=267957&r1=267956&r2=267957&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-shadow.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-shadow.cpp Thu Apr 28 19:37:43 2016
@@ -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 at +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 at +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'}}
More information about the cfe-commits
mailing list