r346041 - Diagnose parameter names that shadow the names of inherited fields under -Wshadow-field.
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 4 16:17:25 PST 2018
Hi Aaron,
Should such changes be reviewed?
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?
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 at 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 at +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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list