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. 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
