Hello! > 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.
yes I agree. > (And if you don't like this warning, then don't enable it.) I want to have warnings when the parameter is unused. I want to enable only those. Personally I would appreciate if there was conservative compiler warnings that I could enable for -Wunused-parameter, -Wsign-compare, .... that only warns when the code is possibly unsafe or there is unused parameter etc. I am not saying we must remove those other warnings. I am fine that they are enabled separately for instance -Wunused-parameter-virtual so I can disable them with -Wno-unused-parameter-virtual. Or move them to the analyser or clang-tidy or something. 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 12 augusti 2015 22:13 Till: David Blaikie Kopia: Daniel Marjamäki; reviews+d11940+public+578c1335b27aa...@reviews.llvm.org; 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 Wed, Aug 12, 2015 at 11:16 AM, David Blaikie <dblai...@gmail.com<mailto:dblai...@gmail.com>> wrote: On Wed, Aug 12, 2015 at 11:12 AM, Nico Weber <tha...@chromium.org<mailto:tha...@chromium.org>> wrote: On Wed, Aug 12, 2015 at 11:06 AM, David Blaikie <dblai...@gmail.com<mailto:dblai...@gmail.com>> wrote: On Wed, Aug 12, 2015 at 10:11 AM, Nico Weber <tha...@chromium.org<mailto:tha...@chromium.org>> wrote: On Tue, Aug 11, 2015 at 10:15 PM, Daniel Marjamäki <daniel.marjam...@evidente.se<mailto: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<tel:%2B46%20%280%29709%2012%2042%2062> E-mail: daniel.marjam...@evidente.se<mailto:daniel.marjam...@evidente.se> www.evidente.se<http://www.evidente.se> ________________________________________ Från: tha...@google.com<mailto:tha...@google.com> [tha...@google.com<mailto:tha...@google.com>] för Nico Weber [tha...@chromium.org<mailto:tha...@chromium.org>] Skickat: den 11 augusti 2015 20:50 Till: David Blaikie Kopia: reviews+d11940+public+578c1335b27aa...@reviews.llvm.org<mailto:reviews%2bd11940%2bpublic%2b578c1335b27aa...@reviews.llvm.org>; Daniel Marjamäki; cfe-commits@lists.llvm.org<mailto: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><mailto: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><mailto: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><mailto: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><mailto: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><mailto: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
