rnk updated this revision to Diff 51697.
rnk added a comment.
- Address review comments, use a DenseMap instead of redoing lookup
http://reviews.llvm.org/D18271
Files:
include/clang/Basic/Diagnostic.h
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
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,18 @@
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) : 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
@@ -9658,6 +9658,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);
}
}
@@ -6361,6 +6362,23 @@
return NewVD;
}
+enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field };
+
+/// Determine what kind of declaration we're shadowing. The order must be
+/// consistent with the %select in the warning message.
+static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl,
+ const DeclContext *OldDC) {
+ if (isa<RecordDecl>(OldDC)) {
+ if (isa<FieldDecl>(ShadowedDecl))
+ return SDK_Field;
+ else
+ return SDK_StaticMember;
+ } else if (OldDC->isFileContext()) {
+ return SDK_Global;
+ }
+ return SDK_Local;
+}
+
/// \brief Diagnose variable or built-in function shadowing. Implements
/// -Wshadow.
///
@@ -6389,11 +6407,18 @@
if (!isa<VarDecl>(ShadowedDecl) && !isa<FieldDecl>(ShadowedDecl))
return;
- // Fields are not shadowed by variables in C++ static methods.
- if (isa<FieldDecl>(ShadowedDecl))
+ if (isa<FieldDecl>(ShadowedDecl)) {
+ // Fields are not shadowed by variables in C++ static methods.
if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewDC))
if (MD->isStatic())
return;
+ // Fields are not shadowed by constructor parameters. This allows a popular
+ // pattern where constructor parameters are named after fields.
+ if (isa<CXXConstructorDecl>(NewDC) && isa<ParmVarDecl>(D)) {
+ ShadowingDecls.insert({D, ShadowedDecl});
+ return;
+ }
+ }
if (VarDecl *shadowedVar = dyn_cast<VarDecl>(ShadowedDecl))
if (shadowedVar->isExternC()) {
@@ -6422,27 +6447,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);
}
@@ -6458,6 +6469,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<DeclRefExpr>(E);
+ if (!DRE)
+ return;
+ const NamedDecl *D = DRE->getDecl();
+ 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(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: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -1652,6 +1652,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,14 @@
"static data member of %2|"
"field of %2}1">,
InGroup<Shadow>, 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<
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
