From 52f027dd7115ad243a927cff142f730dbc539e6f Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Wed, 8 Feb 2017 03:30:13 +0000 Subject: [PATCH] Sema: add warning for c++ member variable shadowing Add a warning for shadowed variables across records. Referencing a shadow'ed variable may not give the desired variable. Add an optional warning for the shadowing. Patch by James Sun! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@294401 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 3 +- include/clang/Basic/DiagnosticSemaKinds.td | 5 + include/clang/Sema/Sema.h | 5 + lib/Sema/SemaDeclCXX.cpp | 55 +++++++++ .../class.derived/class.member.lookup/p10.cpp | 114 ++++++++++++++++++ .../class.derived/class.member.lookup/p6.cpp | 10 +- .../class.derived/class.member.lookup/p7.cpp | 10 +- 7 files changed, 190 insertions(+), 12 deletions(-) create mode 100644 test/CXX/class.derived/class.member.lookup/p10.cpp diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 992f59f6a4..1317e49bc7 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -355,6 +355,7 @@ def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">; def Sentinel : DiagGroup<"sentinel">; def MissingMethodReturnType : DiagGroup<"missing-method-return-type">; +def ShadowField : DiagGroup<"shadow-field">; def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">; def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor", [ShadowFieldInConstructorModified]>; @@ -366,7 +367,7 @@ def ShadowUncapturedLocal : DiagGroup<"shadow-uncaptured-local">; def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified, ShadowIvar]>; def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor, - ShadowUncapturedLocal]>; + ShadowUncapturedLocal, ShadowField]>; def Shorten64To32 : DiagGroup<"shorten-64-to-32">; def : DiagGroup<"sign-promo">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 04a404e104..a25b959da6 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -8959,4 +8959,9 @@ def ext_warn_gnu_final : ExtWarn< "__final is a GNU extension, consider using C++11 final">, InGroup; +def warn_shadow_field : + Warning<"non-static data member '%0' of '%1' shadows member inherited from type '%2'">, + InGroup, DefaultIgnore; +def note_shadow_field : Note<"declared here">; + } // end of sema component. diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 8a28584737..0fb9a5d3d4 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -10074,6 +10074,11 @@ private: void CheckBitFieldInitialization(SourceLocation InitLoc, FieldDecl *Field, Expr *Init); + /// Check if there is a field shadowing. + void CheckShadowInheritedFields(const SourceLocation &Loc, + DeclarationName FieldName, + const CXXRecordDecl *RD); + /// \brief Check if the given expression contains 'break' or 'continue' /// statement that produces control flow different from GCC. void CheckBreakContinueBinding(Expr *E); diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index e3310b60a2..f84d14cfa8 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -2758,6 +2758,56 @@ static AttributeList *getMSPropertyAttr(AttributeList *list) { return nullptr; } +// Check if there is a field shadowing. +void Sema::CheckShadowInheritedFields(const SourceLocation &Loc, + DeclarationName FieldName, + const CXXRecordDecl *RD) { + if (Diags.isIgnored(diag::warn_shadow_field, Loc)) + return; + + // To record a shadowed field in a base + std::map Bases; + auto FieldShadowed = [&](const CXXBaseSpecifier *Specifier, + CXXBasePath &Path) { + const auto Base = Specifier->getType()->getAsCXXRecordDecl(); + // Record an ambiguous path directly + if (Bases.find(Base) != Bases.end()) + return true; + for (const auto Field : Base->lookup(FieldName)) { + if ((isa(Field) || isa(Field)) && + Field->getAccess() != AS_private) { + assert(Field->getAccess() != AS_none); + assert(Bases.find(Base) == Bases.end()); + Bases[Base] = Field; + return true; + } + } + return false; + }; + + CXXBasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/true, + /*DetectVirtual=*/true); + if (!RD->lookupInBases(FieldShadowed, Paths)) + return; + + for (const auto &P : Paths) { + auto Base = P.back().Base->getType()->getAsCXXRecordDecl(); + auto It = Bases.find(Base); + // Skip duplicated bases + if (It == Bases.end()) + continue; + auto BaseField = It->second; + assert(BaseField->getAccess() != AS_private); + if (AS_none != + CXXRecordDecl::MergeAccess(P.Access, BaseField->getAccess())) { + Diag(Loc, diag::warn_shadow_field) + << FieldName.getAsString() << RD->getName() << Base->getName(); + Diag(BaseField->getLocation(), diag::note_shadow_field); + Bases.erase(It); + } + } +} + /// ActOnCXXMemberDeclarator - This is invoked when a C++ class member /// declarator is parsed. 'AS' is the access specifier, 'BW' specifies the /// bitfield width if there is one, 'InitExpr' specifies the initializer if @@ -2957,6 +3007,11 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D, if (!Member) return nullptr; } + + // Check for any possible shadowed member variables + if (const auto *RD = cast(CurContext)) + CheckShadowInheritedFields(Loc, Name, RD); + } else { Member = HandleDeclarator(S, D, TemplateParameterLists); if (!Member) diff --git a/test/CXX/class.derived/class.member.lookup/p10.cpp b/test/CXX/class.derived/class.member.lookup/p10.cpp new file mode 100644 index 0000000000..afd8752188 --- /dev/null +++ b/test/CXX/class.derived/class.member.lookup/p10.cpp @@ -0,0 +1,114 @@ +// RUN: %clang_cc1 -fsyntax-only -verify %s -Wshadow-all + +// Basic cases, ambiguous paths, and fields with different access +class A { +public: + int x; // expected-note 2{{declared here}} +protected: + int y; // expected-note 2{{declared here}} +private: + int z; +}; + +struct B : A { +}; + +struct C : A { +}; + +struct W { + int w; // expected-note {{declared here}} +}; + +struct U : W { +}; + +struct V : W { +}; + +class D { +public: + char w; // expected-note {{declared here}} +private: + char x; +}; + +// Check direct inheritance and multiple paths to the same base. +class E : B, C, D, U, V +{ + unsigned x; // expected-warning {{non-static data member 'x' of 'E' shadows member inherited from type 'A'}} + char y; // expected-warning {{non-static data member 'y' of 'E' shadows member inherited from type 'A'}} + double z; + char w; // expected-warning {{non-static data member 'w' of 'E' shadows member inherited from type 'D'}} expected-warning {{non-static data member 'w' of 'E' shadows member inherited from type 'W'}} +}; + +// Virtual inheritance +struct F : virtual A { +}; + +struct G : virtual A { +}; + +class H : F, G { + int x; // expected-warning {{non-static data member 'x' of 'H' shadows member inherited from type 'A'}} + int y; // expected-warning {{non-static data member 'y' of 'H' shadows member inherited from type 'A'}} + int z; +}; + +// Indirect inheritance +struct I { + union { + int x; // expected-note {{declared here}} + int y; + }; +}; + +struct J : I { + int x; // expected-warning {{non-static data member 'x' of 'J' shadows member inherited from type 'I'}} +}; + +// non-access paths +class N : W { +}; + +struct K { + int y; +}; + +struct L : private K { +}; + +struct M : L { + int y; + int w; +}; + +// Multiple ambiguous paths with different accesses +struct A1 { + int x; // expected-note {{declared here}} +}; + +class B1 : A1 { +}; + +struct B2 : A1 { +}; + +struct C1 : B1, B2 { +}; + +class D1 : C1 { +}; + +struct D2 : C1 { +}; + +class D3 : C1 { +}; + +struct E1 : D1, D2, D3{ + int x; // expected-warning {{non-static data member 'x' of 'E1' shadows member inherited from type 'A1'}} +}; + + + diff --git a/test/CXX/class.derived/class.member.lookup/p6.cpp b/test/CXX/class.derived/class.member.lookup/p6.cpp index 7239881926..0a400a2405 100644 --- a/test/CXX/class.derived/class.member.lookup/p6.cpp +++ b/test/CXX/class.derived/class.member.lookup/p6.cpp @@ -1,24 +1,24 @@ -// RUN: %clang_cc1 -fsyntax-only -verify %s +// RUN: %clang_cc1 -fsyntax-only -verify %s -Wshadow-field class V { public: int f(); - int x; + int x; // expected-note {{declared here}} }; class W { public: int g(); // expected-note{{member found by ambiguous name lookup}} - int y; // expected-note{{member found by ambiguous name lookup}} + int y; // expected-note{{member found by ambiguous name lookup}} expected-note {{declared here}} }; class B : public virtual V, public W { public: int f(); - int x; + int x; // expected-warning {{non-static data member 'x' of 'B' shadows member inherited from type 'V'}} int g(); // expected-note{{member found by ambiguous name lookup}} - int y; // expected-note{{member found by ambiguous name lookup}} + int y; // expected-note{{member found by ambiguous name lookup}} expected-warning {{non-static data member 'y' of 'B' shadows member inherited from type 'W'}} }; class C : public virtual V, public W { }; diff --git a/test/CXX/class.derived/class.member.lookup/p7.cpp b/test/CXX/class.derived/class.member.lookup/p7.cpp index a785e0f90e..775057792c 100644 --- a/test/CXX/class.derived/class.member.lookup/p7.cpp +++ b/test/CXX/class.derived/class.member.lookup/p7.cpp @@ -1,11 +1,9 @@ -// RUN: %clang_cc1 -verify %s +// RUN: %clang_cc1 -verify %s -Wshadow-field -// expected-no-diagnostics - -struct A { int n; }; -struct B { float n; }; +struct A { int n; }; // expected-note {{declared here}} +struct B { float n; }; // expected-note {{declared here}} struct C : A, B {}; struct D : virtual C {}; -struct E : virtual C { char n; }; +struct E : virtual C { char n; }; // expected-warning {{non-static data member 'n' of 'E' shadows member inherited from type 'A'}} expected-warning {{non-static data member 'n' of 'E' shadows member inherited from type 'B'}} struct F : D, E {} f; char &k = f.n; -- 2.40.0