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

[alexfh]

Summary

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

[alexfh]

Ping.

[lebedev.ri]

I would like to see some test in non-strict mode that shows that
it is the child existence is what matters, e.g. with empty lines,
and maybe another one with all the content commented out /**/.

Otherwise this looks good.

clang-tidy/misc/UnusedParametersCheck.cpp

181

So this checks that there are children.

[aaron.ballman]

LGTM aside from some documentation nits.

docs/clang-tidy/checks/misc-unused-parameters.rst

6–13

may signal about a bug -> may signify a bug

42

to hide in -> to hide

[alexfh]

• Addressed review comments.

[alexfh]

clang-tidy/misc/UnusedParametersCheck.cpp

181

Yes, and llvm::distance (recently renamed to llvm::size) doesn't work here any more, so the code now checks for iterator equality, which should be easier to read.

clang-tidy/misc/UnusedParametersCheck.h

 /// Finds unused parameters and fixes them, so that `-Wunused-parameter` can be
 /// turned on.
 class UnusedParametersCheck : public ClangTidyCheck {
 public:
   UnusedParametersCheck(StringRef Name, ClangTidyContext *Context);
   ~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;
 
   void
   warnOnUnusedParameter(const ast_matchers::MatchFinder::MatchResult &Result,
                         const FunctionDecl *Function, unsigned ParamIndex);
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_PARAMETERS_H

clang-tidy/misc/UnusedParametersCheck.cpp

 //===--- UnusedParametersCheck.cpp - clang-tidy----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
 #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;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 bool isOverrideMethod(const FunctionDecl *Function) {
   if (const auto *MD = dyn_cast<CXXMethodDecl>(Function))
     return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
   return false;
 }
 } // 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()))
 }
 
 template <typename T>
 static CharSourceRange removeNode(const MatchFinder::MatchResult &Result,
                                   const T *PrevNode, const T *Node,
                                   const T *NextNode) {
   if (NextNode)
     return CharSourceRange::getCharRange(Node->getLocStart(),
private:
 
   std::unordered_map<const FunctionDecl *, IndexEntry> Index;
 };
 
 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,
     unsigned ParamIndex) {
   const auto *Param = Function->getParamDecl(ParamIndex);
   auto MyDiag = diag(Param->getLocation(), "parameter %0 is unused") << Param;
 
   if (!Indexer) {
void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Function))
     if (Method->isLambdaStaticInvoker())
       return;
   for (unsigned i = 0, e = Function->getNumParams(); i != e; ++i) {
     const auto *Param = Function->getParamDecl(i);
     if (Param->isUsed() || Param->isReferenced() || !Param->getDeclName() ||
         Param->hasAttr<UnusedAttr>())
       continue;
+
+    // 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);
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/misc-unused-parameters.rst

 .. title:: clang-tidy - misc-unused-parameters
 
 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.

test/clang-tidy/misc-unused-parameters-strict.cpp

+// 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() {}{{$}}
+};
+}

test/clang-tidy/misc-unused-parameters.cpp

 
 static void dontGetConfusedByParametersInFunctionTypes() { void (*F)(int i); }
 
 template <typename T> class Function {};
 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