SV: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method

[Daniel Marjamäki via cfe-commits]

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.
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&#246;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