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:56:22 PST 2018
> 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!
>
> ~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