On Tue, Dec 4, 2018 at 7:56 PM George Karpenkov <ekarpen...@apple.com> wrote: > > > > > On Dec 4, 2018, at 4:48 PM, Aaron Ballman <aa...@aaronballman.com> wrote: > > > > 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. > > OK thanks!
I've put a patch up for review at: https://reviews.llvm.org/D55321 and made you a reviewer. ~Aaron > > > > > ~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
