[clang-tidy] misc-unused-parameters - retain old behavior under StrictMode

Summary: This addresses https://bugs.llvm.org/show_bug.cgi?id=37467.

Reviewers: klimek, ilya-biryukov, lebedev.ri, aaron.ballman

Reviewed By: lebedev.ri, aaron.ballman

Subscribers: aaron.ballman, lebedev.ri, xazax.hun, cfe-commits

Differential Revision: https://reviews.llvm.org/D46951

llvm-svn: 335863

diff --git a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
index e09f514..9f5f4da 100644 (file)
--- a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/STLExtras.h"
 #include <unordered_set>
 
 using namespace clang::ast_matchers;
@@ -29,11 +30,10 @@ bool isOverrideMethod(const FunctionDecl *Function) {
 } // namespace
 
 void UnusedParametersCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(isDefinition(),
-                                  hasBody(stmt(hasDescendant(stmt()))),
-                                  hasAnyParameter(decl()))
-                         .bind("function"),
-                     this);
+  Finder->addMatcher(
+      functionDecl(isDefinition(), hasBody(stmt()), hasAnyParameter(decl()))
+          .bind("function"),
+      this);
 }
 
 template <typename T>
@@ -122,7 +122,12 @@ UnusedParametersCheck::~UnusedParametersCheck() = default;
 
 UnusedParametersCheck::UnusedParametersCheck(StringRef Name,
                                              ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context) {}
+    : ClangTidyCheck(Name, Context),
+      StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0) {}
+
+void UnusedParametersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StrictMode", StrictMode);
+}
 
 void UnusedParametersCheck::warnOnUnusedParameter(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function,
@@ -170,7 +175,15 @@ void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
     if (Param->isUsed() || Param->isReferenced() || !Param->getDeclName() ||
         Param->hasAttr<UnusedAttr>())
       continue;
-    warnOnUnusedParameter(Result, Function, i);
+
+    // In non-strict mode ignore function definitions with empty bodies
+    // (constructor initializer counts for non-empty body).
+    if (StrictMode ||
+        (Function->getBody()->child_begin() !=
+         Function->getBody()->child_end()) ||
+        (isa<CXXConstructorDecl>(Function) &&
+         cast<CXXConstructorDecl>(Function)->getNumCtorInitializers() > 0))
+      warnOnUnusedParameter(Result, Function, i);
   }
 }
 

diff --git a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.h b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.h
index 414ef11..b9bae26 100644 (file)
--- a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.h
@@ -24,8 +24,10 @@ public:
   ~UnusedParametersCheck();
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
+  const bool StrictMode;
   class IndexerVisitor;
   std::unique_ptr<IndexerVisitor> Indexer;
 

diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
index de868b9..3dfeb29 100644 (file)
--- a/clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -3,24 +3,40 @@
 misc-unused-parameters
 ======================
 
-Finds unused parameters and fixes them, so that `-Wunused-parameter` can be
-turned on.
+Finds unused function parameters. Unused parameters may signify a bug in the
+code (e.g. when a different parameter is used instead). The suggested fixes
+either comment parameter name out or remove the parameter completely, if all
+callers of the function are in the same translation unit and can be updated.
+
+The check is similar to the `-Wunused-parameter` compiler diagnostic and can be
+used to prepare a codebase to enabling of that diagnostic. By default the check
+is more permissive (see :option:`StrictMode`).
 
 .. code-block:: c++
 
-  void a(int i) {}
+  void a(int i) { /*some code that doesn't use `i`*/ }
 
   // becomes
 
-  void a(int  /*i*/) {}
-
+  void a(int  /*i*/) { /*some code that doesn't use `i`*/ }
 
 .. code-block:: c++
 
   static void staticFunctionA(int i);
-  static void staticFunctionA(int i) {}
+  static void staticFunctionA(int i) { /*some code that doesn't use `i`*/ }
 
   // becomes
 
   static void staticFunctionA()
-  static void staticFunctionA() {}
+  static void staticFunctionA() { /*some code that doesn't use `i`*/ }
+
+Options
+-------
+
+.. option:: StrictMode
+
+   When zero (default value), the check will ignore trivially unused parameters,
+   i.e. when the corresponding function has an empty body (and in case of
+   constructors - no constructor initializers). When the function body is empty,
+   an unused parameter is unlikely to be unnoticed by a human reader, and
+   there's basically no place for a bug to hide.

diff --git a/clang-tools-extra/test/clang-tidy/misc-unused-parameters-strict.cpp b/clang-tools-extra/test/clang-tidy/misc-unused-parameters-strict.cpp
new file mode 100644 (file)
index 0000000..a334b45
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/misc-unused-parameters-strict.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s misc-unused-parameters %t -- \
+// RUN:   -config="{CheckOptions: [{key: StrictMode, value: 1}]}" --
+
+// Warn on empty function bodies in StrictMode.
+namespace strict_mode {
+void f(int foo) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: parameter 'foo' is unused [misc-unused-parameters]
+// CHECK-FIXES: {{^}}void f(int  /*foo*/) {}{{$}}
+class E {
+  int i;
+
+public:
+  E(int j) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  E(int  /*j*/) {}{{$}}
+};
+class F {
+  int i;
+
+public:
+  F(int j) : i() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  F(int  /*j*/) : i() {}{{$}}
+};
+}

diff --git a/clang-tools-extra/test/clang-tidy/misc-unused-parameters.cpp b/clang-tools-extra/test/clang-tidy/misc-unused-parameters.cpp
index 42554b9..feb3a14 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/misc-unused-parameters.cpp
+++ b/clang-tools-extra/test/clang-tidy/misc-unused-parameters.cpp
@@ -222,5 +222,41 @@ static Function<void(int, int i)> dontGetConfusedByFunctionReturnTypes() {
   return Function<void(int, int)>();
 }
 
+namespace strict_mode_off {
 // Do not warn on empty function bodies.
-void f(int foo) {}
+void f1(int foo1) {}
+void f2(int foo2) {
+  // "empty" in the AST sense, not in textual sense.
+}
+void f3(int foo3) {;}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: parameter 'foo3' is unused
+// CHECK-FIXES: {{^}}void f3(int  /*foo3*/) {;}{{$}}
+
+class E {
+  int i;
+
+public:
+  E(int j) {}
+};
+class F {
+  int i;
+
+public:
+  // Constructor initializer counts as a non-empty body.
+  F(int j) : i() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'j' is unused
+// CHECK-FIXES: {{^}}  F(int  /*j*/) : i() {}{{$}}
+};
+
+class A {
+public:
+  A();
+  A(int);
+};
+class B : public A {
+public:
+  B(int i) : A() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is unused
+// CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
+};
+} // namespace strict_mode_off