[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 11:12:16 PDT 2015
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.
>
> 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/102e0825/attachment.html>
More information about the cfe-commits
mailing list