r346041 - Diagnose parameter names that shadow the names of inherited fields under -Wshadow-field.
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 4 16:48:53 PST 2018
On Tue, Dec 4, 2018 at 7:17 PM George Karpenkov <ekarpenkov at 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 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