commit 7db75683e3b3182eb878377c4fcc445cd99eb80f Author: James Sun Date: Tue Jan 24 14:08:45 2017 -0800 Add warning for c++ member variable shadowing For cases where we really don't want to shadow member variables, e.g. struct a { int foo; // A public or protected field } class b : a { int foo; // Generate a warning } This patch (1) adds a member variable shadowing checking, and (2) incorporates it to the unit tests. diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 6290172..3103200 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -8926,4 +8926,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; +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 1d3b273..ce10232 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -10061,6 +10061,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 a70e16c..5e2e7f0 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -2758,6 +2758,53 @@ 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(); + if (Bases.find(Base) != Bases.end()) + // Record an ambiguous path directly + 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); + if (It == Bases.end()) + // Skip duplicated bases + 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 +3004,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/Analysis/padding_cpp.cpp b/test/Analysis/padding_cpp.cpp index df2f2a8..b4a9421 100644 --- a/test/Analysis/padding_cpp.cpp +++ b/test/Analysis/padding_cpp.cpp @@ -4,17 +4,17 @@ #include "padding_c.c" struct BigCharArray2 { // no-warning - char c[129]; + char c[129]; // expected-note 2{{declared here}} }; // xxxexpected-warning@+1{{Excessive padding in 'struct LowAlignmentBase'}} struct LowAlignmentBase : public BigCharArray2 { int i; - char c; + char c; // expected-warning{{non-static data member 'c' of 'LowAlignmentBase' shadows member inherited from type 'BigCharArray2'}} }; struct CorrectLowAlignmentBase : public BigCharArray2 { // no-warning - char c; + char c; // expected-warning{{non-static data member 'c' of 'CorrectLowAlignmentBase' shadows member inherited from type 'BigCharArray2'}} int i; }; 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 0000000..dbdfb3b --- /dev/null +++ b/test/CXX/class.derived/class.member.lookup/p10.cpp @@ -0,0 +1,114 @@ +// RUN: %clang_cc1 -fsyntax-only -verify %s + +// 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 7239881..575f670 100644 --- a/test/CXX/class.derived/class.member.lookup/p6.cpp +++ b/test/CXX/class.derived/class.member.lookup/p6.cpp @@ -3,22 +3,22 @@ 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 a785e0f..c4d53d9 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 -// 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; diff --git a/test/CXX/dcl.decl/dcl.decomp/p4.cpp b/test/CXX/dcl.decl/dcl.decomp/p4.cpp index c461eb6..7556763 100644 --- a/test/CXX/dcl.decl/dcl.decomp/p4.cpp +++ b/test/CXX/dcl.decl/dcl.decomp/p4.cpp @@ -6,7 +6,7 @@ template struct same { ~same(); }; struct Empty {}; struct A { - int a; + int a; // expected-note {{declared here}} }; namespace NonPublicMembers { @@ -53,7 +53,7 @@ namespace AnonymousMember { namespace MultipleClasses { struct B : A { - int a; + int a; // expected-warning {{non-static data member 'a' of 'B' shadows member inherited from type 'A'}} }; struct C { int a; }; diff --git a/test/CXX/drs/dr5xx.cpp b/test/CXX/drs/dr5xx.cpp index 89e404f..ff6cf2a 100644 --- a/test/CXX/drs/dr5xx.cpp +++ b/test/CXX/drs/dr5xx.cpp @@ -653,9 +653,9 @@ namespace dr566 { // dr566: yes namespace dr568 { // dr568: yes c++11 // FIXME: This is a DR issue against C++98, so should probably apply there // too. - struct x { int y; }; + struct x { int y; }; // expected-note {{declared here}} class trivial : x { - x y; + x y; // expected-warning {{non-static data member 'y' of 'trivial' shadows member inherited from type 'x'}} public: int n; }; diff --git a/test/SemaCXX/class-layout.cpp b/test/SemaCXX/class-layout.cpp index 96552de..8b3b947 100644 --- a/test/SemaCXX/class-layout.cpp +++ b/test/SemaCXX/class-layout.cpp @@ -1,6 +1,5 @@ // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -// expected-no-diagnostics #define SA(n, p) int a##n[(p) ? 1 : -1] @@ -66,10 +65,10 @@ SA(7, sizeof(A) == 1); class B : A { bool iv0 : 1; }; SA(8, sizeof(B) == 2); -struct C { bool iv0 : 1; }; +struct C { bool iv0 : 1; }; // expected-note {{declared here}} SA(9, sizeof(C) == 1); -struct D : C { bool iv0 : 1; }; +struct D : C { bool iv0 : 1; }; // expected-warning {{non-static data member 'iv0' of 'D' shadows member inherited from type 'C'}} SA(10, sizeof(D) == 2); } @@ -501,7 +500,7 @@ namespace test17 { }; struct A { - pod_in_11_only a __attribute__((aligned(512))); + pod_in_11_only a __attribute__((aligned(512))); // expected-note {{declared here}} }; struct B { @@ -519,7 +518,7 @@ namespace test17 { }; struct might_use_tail_padding : public A, public B, public C, public D { - char a; + char a; // expected-warning {{non-static data member 'a' of 'might_use_tail_padding' shadows member inherited from type 'A'}} }; SA(0, sizeof(might_use_tail_padding) == 512); } diff --git a/test/SemaCXX/constant-expression-cxx11.cpp b/test/SemaCXX/constant-expression-cxx11.cpp index 0668324..68f7109 100644 --- a/test/SemaCXX/constant-expression-cxx11.cpp +++ b/test/SemaCXX/constant-expression-cxx11.cpp @@ -26,9 +26,9 @@ struct MemberZero { namespace DerivedToVBaseCast { - struct U { int n; }; - struct V : U { int n; }; - struct A : virtual V { int n; }; + struct U { int n; }; // expected-note {{declared here}} + struct V : U { int n; }; // expected-warning {{non-static data member 'n' of 'V' shadows member inherited from type 'U'}} expected-note {{declared here}} + struct A : virtual V { int n; }; // expected-warning {{non-static data member 'n' of 'A' shadows member inherited from type 'V'}} struct Aa { int n; }; struct B : virtual A, Aa {}; struct C : virtual A, Aa {};