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