rnk updated this revision to Diff 52982.
rnk marked 3 inline comments as done.
rnk added a comment.
- Add -Wshadow-all and -Wshadow-field-in-constructor, also address review
comments
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
@@ -29,7 +29,19 @@
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) // 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'}}
+ }
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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
