> 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! > > ~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
