[PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 12 13:13:13 PDT 2015
On Wed, Aug 12, 2015 at 11:16 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Wed, Aug 12, 2015 at 11:12 AM, Nico Weber <thakis at chromium.org> wrote:
>
>> On Wed, Aug 12, 2015 at 11:06 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Wed, Aug 12, 2015 at 10:11 AM, Nico Weber <thakis at chromium.org>
>>> wrote:
>>>
>>>> On Tue, Aug 11, 2015 at 10:15 PM, Daniel Marjamäki <
>>>> Daniel.Marjamaki at evidente.se> wrote:
>>>>
>>>>>
>>>>> ideally there should be no -Wunused-parameter compiler warning when
>>>>> the parameter is used.
>>>>>
>>>>> would it feel better to move the "FP" warnings about virtual
>>>>> functions, for instance to clang-tidy?
>>>>>
>>>>> > If you enable this warning, you probably want to know about unused
>>>>> parameters, independent of if your function is virtual or not, no?
>>>>>
>>>>> imho there are some compiler warnings that are too noisy. I don't like
>>>>> to get a warning when there is obviously no bug:
>>>>>
>>>>> sign compare:
>>>>> if the signed value is obviously not negative then there is no bug:
>>>>> signed x; .. if (x>10 && x < s.size())
>>>>> unused parameter:
>>>>> could check in the current translation unit if the parameter is used
>>>>> in an overloaded method.
>>>>>
>>>>
>>>> It doesn't warn about the presence of the parameter, but about the
>>>> presence of the name. If you say f(int, int) instead of f(int a, int b)
>>>> then the warning won't fire.
>>>>
>>>
>>>
>>>> (And if you don't like this warning, then don't enable it.)
>>>>
>>>
>>> This isn't usually the approach we take with Clang's warnings - we try
>>> to remove false positives (where "false positive" is usually defined as
>>> "diagnoses something which is not a bug" (where bug is defined as "the
>>> resulting program behaves in a way that the user doesn't intend/expect"))
>>> where practical.
>>>
>>
>> Sure, for warnings that are supposed to find bugs. The -Wunused warnings
>> warn about stuff that's unused, not bugs.
>>
>
> Seems a reasonable analog here, though, would be that a true positive for
> -Wunused is when the thing really is unused and should be removed.
> Commenting out the variable name is the suppression mechanism to workaround
> false positives.
>
I disagree with this assessment. The warning warns about unused parameter
names. So this is a true positive.
> If there's a targetable subset of cases where the s/n is low enough, it
> could be reasonable to suppress the warning in that subset, I think.
>
> (see improvements to -Wunreachable-code to suppress cases that are
> unreachable in this build (sizeof(int) == 4 conditions, macros, etc), or
> represent valid defensive programming (default in a covered enum switch) to
> make the diagnostic more useful/less noisy)
>
>
>>
>>
>>>
>>> If the subset of cases where this warning fires on parameters to virtual
>>> functions produces more noise (if a significant % of cases just result in
>>> commenting out the parameter name rather than removing the parameter) than
>>> signal, it's certainly within the realm of discussion that we have about
>>> whether that subset of cases is worth keeping in the warning.
>>>
>>> - David
>>>
>>>
>>>>
>>>>
>>>>> constructor initialization order:
>>>>> should not warn if the order obviously does not matter. for instance
>>>>> initialization order of pod variables using constants.
>>>>> etc
>>>>>
>>>>> Best regards,
>>>>> Daniel Marjamäki
>>>>>
>>>>>
>>>>> ..................................................................................................................
>>>>> Daniel Marjamäki Senior Engineer
>>>>> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>>>>>
>>>>> Mobile: +46 (0)709 12 42 62
>>>>> E-mail: Daniel.Marjamaki at evidente.se
>>>>>
>>>>> www.evidente.se
>>>>>
>>>>> ________________________________________
>>>>> Från: thakis at google.com [thakis at google.com] för Nico Weber [
>>>>> thakis at chromium.org]
>>>>> Skickat: den 11 augusti 2015 20:50
>>>>> Till: David Blaikie
>>>>> Kopia: reviews+D11940+public+578c1335b27aa98b at reviews.llvm.org;
>>>>> Daniel Marjamäki; cfe-commits at lists.llvm.org
>>>>> Ämne: Re: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual
>>>>> method or method that overrides base class method
>>>>>
>>>>> On Tue, Aug 11, 2015 at 11:32 AM, David Blaikie <dblaikie at gmail.com
>>>>> <mailto:dblaikie at gmail.com>> wrote:
>>>>>
>>>>>
>>>>> On Tue, Aug 11, 2015 at 8:46 AM, Nico Weber via cfe-commits <
>>>>> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
>>>>> Can't you just change your signature to
>>>>>
>>>>> virtual void a(int /* x */) {}
>>>>>
>>>>> in these cases?
>>>>>
>>>>> You could - does it add much value to do that, though?
>>>>>
>>>>> If you enable this warning, you probably want to know about unused
>>>>> parameters, independent of if your function is virtual or not, no?
>>>>>
>>>>> (perhaps it does - it means you express the intent that the parameter
>>>>> is not used and the compiler helps you check that (so that for the
>>>>> parameters you think /should/ be used (you haven't commented out their name
>>>>> but accidentally shadow or otherwise fail to reference, you still get a
>>>>> warning))
>>>>>
>>>>> - David
>>>>>
>>>>>
>>>>> On Tue, Aug 11, 2015 at 6:43 AM, Daniel Marjamäki <
>>>>> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
>>>>> danielmarjamaki created this revision.
>>>>> danielmarjamaki added a reviewer: krememek.
>>>>> danielmarjamaki added a subscriber: cfe-commits.
>>>>>
>>>>> Don't diagnose -Wunused-parameter in methods that override other
>>>>> methods because the overridden methods might use the parameter
>>>>>
>>>>> Don't diagnose -Wunused-parameter in virtual methods because these
>>>>> might be overriden by other methods that use the parameter.
>>>>>
>>>>> Such diagnostics could be more accurately written if they are based on
>>>>> whole-program-analysis that establish if such parameter is unused in all
>>>>> methods.
>>>>>
>>>>>
>>>>>
>>>>> http://reviews.llvm.org/D11940
>>>>>
>>>>> Files:
>>>>> lib/Sema/SemaDecl.cpp
>>>>> test/SemaCXX/warn-unused-parameters.cpp
>>>>>
>>>>> Index: test/SemaCXX/warn-unused-parameters.cpp
>>>>> ===================================================================
>>>>> --- test/SemaCXX/warn-unused-parameters.cpp
>>>>> +++ test/SemaCXX/warn-unused-parameters.cpp
>>>>> @@ -32,3 +32,20 @@
>>>>> auto l = [&t...]() { return sizeof...(s); };
>>>>> return l();
>>>>> }
>>>>> +
>>>>> +// Don't diagnose virtual methods or methods that override base class
>>>>> +// methods.
>>>>> +class Base {
>>>>> +public:
>>>>> + virtual void f(int x);
>>>>> +};
>>>>> +
>>>>> +class Derived : public Base {
>>>>> +public:
>>>>> + // Don't warn in overridden methods.
>>>>> + virtual void f(int x) {}
>>>>> +
>>>>> + // Don't warn in virtual methods.
>>>>> + virtual void a(int x) {}
>>>>> +};
>>>>> +
>>>>> Index: lib/Sema/SemaDecl.cpp
>>>>> ===================================================================
>>>>> --- lib/Sema/SemaDecl.cpp
>>>>> +++ lib/Sema/SemaDecl.cpp
>>>>> @@ -10797,8 +10797,13 @@
>>>>>
>>>>> if (!FD->isInvalidDecl()) {
>>>>> // Don't diagnose unused parameters of defaulted or deleted
>>>>> functions.
>>>>> - if (!FD->isDeleted() && !FD->isDefaulted())
>>>>> - DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
>>>>> + if (!FD->isDeleted() && !FD->isDefaulted()) {
>>>>> + // Don't diagnose unused parameters in virtual methods or
>>>>> + // in methods that override base class methods.
>>>>> + const auto MD = dyn_cast<CXXMethodDecl>(FD);
>>>>> + if (!MD || (MD->size_overridden_methods() == 0U &&
>>>>> !MD->isVirtual()))
>>>>> + DiagnoseUnusedParameters(FD->param_begin(),
>>>>> FD->param_end());
>>>>> + }
>>>>> DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(),
>>>>> FD->param_end(),
>>>>> FD->getReturnType(), FD);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150812/6597cc54/attachment-0001.html>
More information about the cfe-commits
mailing list