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
Wed Dec 5 06:49:44 PST 2018
On Tue, Dec 4, 2018 at 7:56 PM George Karpenkov <ekarpenkov at apple.com> wrote:
>
>
>
> > On Dec 4, 2018, at 4:48 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> >
> > 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.
>
> 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 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