On Tue, Dec 4, 2018 at 7:17 PM George Karpenkov <ekarpen...@apple.com> wrote: > > Hi Aaron, > > Should such changes be reviewed?
This was reviewed in D52421. > In particular, it introduces tons of warnings in XNU (which are painful due > to -Werror). > I expect similar issues in many other our projects. > > While in principle correct, > wouldn’t it make more sense to only warn on function definitions and not > declarations? Absolutely! I'll make that change, thank you for the suggestion. ~Aaron > > This would avoid: > > - A gigantic avalanche of warnings when compiling a project, > as the warning would be duplicated for every TU including the problematic > header > - A warning on declaration without a body is useful: nothing can collide > there, > and the actual definition might use a different name. > > George > > > On Nov 2, 2018, at 2:04 PM, Aaron Ballman via cfe-commits > > <cfe-commits@lists.llvm.org> wrote: > > > > Author: aaronballman > > Date: Fri Nov 2 14:04:44 2018 > > New Revision: 346041 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=346041&view=rev > > Log: > > Diagnose parameter names that shadow the names of inherited fields under > > -Wshadow-field. > > > > This addresses PR34120. Note, unlike GCC, we take into account the > > accessibility of the field when deciding whether to warn or not. > > > > Modified: > > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > > cfe/trunk/include/clang/Sema/Sema.h > > cfe/trunk/lib/Sema/SemaDecl.cpp > > cfe/trunk/lib/Sema/SemaDeclCXX.cpp > > cfe/trunk/test/SemaCXX/warn-shadow.cpp > > > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=346041&r1=346040&r2=346041&view=diff > > ============================================================================== > > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Nov 2 > > 14:04:44 2018 > > @@ -9467,16 +9467,15 @@ def warn_block_literal_qualifiers_on_omi > > InGroup<IgnoredQualifiers>; > > > > def ext_warn_gnu_final : ExtWarn< > > - "__final is a GNU extension, consider using C++11 final">, > > - InGroup<GccCompat>; > > - > > -def warn_shadow_field : > > - Warning<"non-static data member %0 of %1 shadows member inherited from " > > - "type %2">, > > - InGroup<ShadowField>, DefaultIgnore; > > -def note_shadow_field : Note<"declared here">; > > - > > -def err_multiversion_required_in_redecl : Error< > > + "__final is a GNU extension, consider using C++11 final">, > > + InGroup<GccCompat>; > > + > > +def warn_shadow_field : Warning< > > + "%select{parameter|non-static data member}3 %0 %select{|of %1 }3shadows " > > + "member inherited from type %2">, InGroup<ShadowField>, DefaultIgnore; > > +def note_shadow_field : Note<"declared here">; > > + > > +def err_multiversion_required_in_redecl : Error< > > "function declaration is missing %select{'target'|'cpu_specific' or " > > "'cpu_dispatch'}0 attribute in a multiversioned function">; > > def note_multiversioning_caused_here : Note< > > > > Modified: cfe/trunk/include/clang/Sema/Sema.h > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=346041&r1=346040&r2=346041&view=diff > > ============================================================================== > > --- cfe/trunk/include/clang/Sema/Sema.h (original) > > +++ cfe/trunk/include/clang/Sema/Sema.h Fri Nov 2 14:04:44 2018 > > @@ -10588,7 +10588,8 @@ private: > > /// Check if there is a field shadowing. > > void CheckShadowInheritedFields(const SourceLocation &Loc, > > DeclarationName FieldName, > > - const CXXRecordDecl *RD); > > + const CXXRecordDecl *RD, > > + bool DeclIsField = true); > > > > /// Check if the given expression contains 'break' or 'continue' > > /// statement that produces control flow different from GCC. > > > > Modified: cfe/trunk/lib/Sema/SemaDecl.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=346041&r1=346040&r2=346041&view=diff > > ============================================================================== > > --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) > > +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Nov 2 14:04:44 2018 > > @@ -12366,6 +12366,13 @@ Decl *Sema::ActOnParamDeclarator(Scope * > > D.setInvalidType(true); > > } > > } > > + > > + if (LangOpts.CPlusPlus) { > > + DeclarationNameInfo DNI = GetNameForDeclarator(D); > > + if (auto *RD = dyn_cast<CXXRecordDecl>(CurContext)) > > + CheckShadowInheritedFields(DNI.getLoc(), DNI.getName(), RD, > > + /*DeclIsField*/ false); > > + } > > } > > > > // Temporarily put parameter variables in the translation unit, not > > > > Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=346041&r1=346040&r2=346041&view=diff > > ============================================================================== > > --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) > > +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Fri Nov 2 14:04:44 2018 > > @@ -2832,13 +2832,14 @@ static const ParsedAttr *getMSPropertyAt > > 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; > > - > > +// Check if there is a field shadowing. > > +void Sema::CheckShadowInheritedFields(const SourceLocation &Loc, > > + DeclarationName FieldName, > > + const CXXRecordDecl *RD, > > + bool DeclIsField) { > > + if (Diags.isIgnored(diag::warn_shadow_field, Loc)) > > + return; > > + > > // To record a shadowed field in a base > > std::map<CXXRecordDecl*, NamedDecl*> Bases; > > auto FieldShadowed = [&](const CXXBaseSpecifier *Specifier, > > @@ -2872,13 +2873,13 @@ void Sema::CheckShadowInheritedFields(co > > 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 << RD << Base; > > - Diag(BaseField->getLocation(), diag::note_shadow_field); > > - Bases.erase(It); > > - } > > + if (AS_none != > > + CXXRecordDecl::MergeAccess(P.Access, BaseField->getAccess())) { > > + Diag(Loc, diag::warn_shadow_field) > > + << FieldName << RD << Base << DeclIsField; > > + Diag(BaseField->getLocation(), diag::note_shadow_field); > > + Bases.erase(It); > > + } > > } > > } > > > > > > Modified: cfe/trunk/test/SemaCXX/warn-shadow.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-shadow.cpp?rev=346041&r1=346040&r2=346041&view=diff > > ============================================================================== > > --- cfe/trunk/test/SemaCXX/warn-shadow.cpp (original) > > +++ cfe/trunk/test/SemaCXX/warn-shadow.cpp Fri Nov 2 14:04:44 2018 > > @@ -59,13 +59,13 @@ class A { > > // expected-warning-re@+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'}} > > - char *data; // expected-warning {{declaration shadows a static data > > member of 'A'}} > > + void test() { > > + char *field; // expected-warning {{declaration shadows a field of 'A'}} > > + char *data; // expected-warning {{declaration shadows a static data > > member of 'A'}} > > char *a1; // no warning > > - char *a2; // no warning > > - char *jj; // no warning > > - char *jjj; // no warning > > + char *a2; // no warning > > + char *jj; // no warning > > + char *jjj; // no warning > > } > > > > void test2() { > > @@ -196,14 +196,14 @@ void avoidWarningWhenRedefining(int b) { > > int k; // expected-note {{previous definition is here}} > > typedef int k; // expected-error {{redefinition of 'k'}} > > > > - using l=char; // no warning or error. > > - using l=char; // no warning or error. > > - typedef char l; // no warning or error. > > + using l=char; // no warning or error. > > + using l=char; // no warning or error. > > + typedef char l; // no warning or error. > > > > typedef char n; // no warning or error. > > - typedef char n; // no warning or error. > > - using n=char; // no warning or error. > > -} > > + typedef char n; // no warning or error. > > + using n=char; // no warning or error. > > +} > > > > } > > > > @@ -219,9 +219,37 @@ void f(int a) { > > struct A { > > void g(int a) {} > > A() { int a; } > > - }; > > -} > > -} > > + }; > > +} > > +} > > + > > +namespace PR34120 { > > +struct A { > > + int B; // expected-note {{declared here}} > > +}; > > + > > +class C : public A { > > + void D(int B) {} // expected-warning {{parameter 'B' shadows member > > inherited from type 'A'}} > > + void E() { > > + extern void f(int B); // Ok > > + } > > +}; > > + > > +class Private { > > + int B; > > +}; > > +class Derived : Private { > > + void D(int B) {} // Ok > > +}; > > + > > +struct Static { > > + static int B; > > +}; > > + > > +struct Derived2 : Static { > > + void D(int B) {} > > +}; > > +} > > > > int PR24718; > > enum class X { PR24718 }; // Ok, not shadowing > > > > > > _______________________________________________ > > cfe-commits mailing list > > cfe-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
