On Wed, Aug 12, 2015 at 11:16 AM, David Blaikie <dblai...@gmail.com> wrote:
> > > On Wed, Aug 12, 2015 at 11:12 AM, Nico Weber <tha...@chromium.org> wrote: > >> On Wed, Aug 12, 2015 at 11:06 AM, David Blaikie <dblai...@gmail.com> >> wrote: >> >>> >>> >>> On Wed, Aug 12, 2015 at 10:11 AM, Nico Weber <tha...@chromium.org> >>> wrote: >>> >>>> On Tue, Aug 11, 2015 at 10:15 PM, Daniel Marjamäki < >>>> daniel.marjam...@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.marjam...@evidente.se >>>>> >>>>> www.evidente.se >>>>> >>>>> ________________________________________ >>>>> Från: tha...@google.com [tha...@google.com] för Nico Weber [ >>>>> tha...@chromium.org] >>>>> Skickat: den 11 augusti 2015 20:50 >>>>> Till: David Blaikie >>>>> Kopia: reviews+d11940+public+578c1335b27aa...@reviews.llvm.org; >>>>> Daniel Marjamäki; cfe-commits@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 <dblai...@gmail.com >>>>> <mailto:dblai...@gmail.com>> wrote: >>>>> >>>>> >>>>> On Tue, Aug 11, 2015 at 8:46 AM, Nico Weber via cfe-commits < >>>>> cfe-commits@lists.llvm.org<mailto:cfe-commits@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@lists.llvm.org<mailto:cfe-commits@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@lists.llvm.org<mailto:cfe-commits@lists.llvm.org> >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org> >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
