[clang-tidy] Added IgnoreVirtual option to misc-unused-parameters

[PiotrZSL]

Summary

Added option to ignore virtual methods in this check.
This allows to quickly get rid of big number of
potentialy false-positive issues without inserting
not-needed comments.

Fixes #55665.

[carlosgalvezp]

Looks good, small comments!

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

185

Since these 2 conditions are unrelated, I believe it's better to put them in separate ifs:

if (IgnoreVirtual && Method->isVirtual())
  return;

if (Method->isLambdaStaticInvoker())
  return;

clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters-virtual.cpp

5

We typically call it Base. Class can also be confusing since it's a struct ;)

[njames93]

Have you thought about handling CRTP overrides

template <typename T>
class Base {
  int getThing(int x) {
    return x;
  }
};

class Derived : public Base<Derived> {
  int getThing(int x) {
    return 0;
  }
};

I'm not saying update this to work for that, but could be a good direction in the future.

clang-tools-extra/docs/ReleaseNotes.rst

225

Don't really need to specify the default here, that's what the check documentation is for

310–311

Please revert this change, it's unrelated.

clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters-virtual.cpp

8

The Regex line start and end markers aren't necessary

18

Small nit: Insert override here so it's obvious to readers of the test that this is a virtual function

[PiotrZSL]

Review fixes

[PiotrZSL]

CRTP not planed.
Not in scope of #55665.

[njames93]

LGTM.

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.h

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;
+  const bool IgnoreVirtual;
   class IndexerVisitor;
   std::unique_ptr<IndexerVisitor> Indexer;
 
   void
   warnOnUnusedParameter(const ast_matchers::MatchFinder::MatchResult &Result,
                         const FunctionDecl *Function, unsigned ParamIndex);
 };
 
 } // namespace clang::tidy::misc
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_PARAMETERS_H

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

private:
   std::unordered_map<const FunctionDecl *, IndexEntry> Index;
 };
 
 UnusedParametersCheck::~UnusedParametersCheck() = default;
 
 UnusedParametersCheck::UnusedParametersCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      StrictMode(Options.getLocalOrGlobal("StrictMode", false)) {}
+      StrictMode(Options.getLocalOrGlobal("StrictMode", false)),
+      IgnoreVirtual(Options.get("IgnoreVirtual", false)) {}
 
 void UnusedParametersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "StrictMode", StrictMode);
+  Options.store(Opts, "IgnoreVirtual", IgnoreVirtual);
 }
 
 void UnusedParametersCheck::warnOnUnusedParameter(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function,
     unsigned ParamIndex) {
   const auto *Param = Function->getParamDecl(ParamIndex);
   // Don't bother to diagnose invalid parameters as being unused.
   if (Param->isInvalidDecl())
for (const CallExpr *Call : Indexer->getFnCalls(Function))
     if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
       MyDiag << removeArgument(Result, Call, ParamIndex);
 }
 
 void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function");
   if (!Function->hasWrittenPrototype() || Function->isTemplateInstantiation())
     return;
-  if (const auto *Method = dyn_cast<CXXMethodDecl>(Function))
+  if (const auto *Method = dyn_cast<CXXMethodDecl>(Function)) {
+    if (IgnoreVirtual && Method->isVirtual())
+      return;
     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).

clang-tools-extra/docs/ReleaseNotes.rst

- Deprecated check-local options `HeaderFileExtensions`
   in :doc:`misc-definitions-in-headers
   <clang-tidy/checks/misc/definitions-in-headers>` check.
   Global options of the same name should be used instead.
 
 - Fixed false positive in :doc:`misc-definitions-in-headers
   <clang-tidy/checks/misc/definitions-in-headers>` to avoid warning on
   declarations inside anonymous namespaces.
 
+- Improved :doc:`misc-unused-parameters
+  <clang-tidy/checks/misc/unused-parameters>` check with new `IgnoreVirtual`
+  option to optionally ignore virtual methods.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`misc-unused-using-decls
   <clang-tidy/checks/misc/unused-using-decls>` check.
   Global options of the same name should be used instead.
 
 - In :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` count template
   constructors toward hand written constructors so that they are skipped if more
- Fixed an issue in :doc:`readability-identifier-naming
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
 - Improved :doc:`modernize-concat-nested-namespaces
   <clang-tidy/checks/modernize/concat-nested-namespaces>` to fix incorrect fixes when 
   using macro between namespace declarations and false positive when using namespace 
   with attributes.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
 Removed checks
 ^^^^^^^^^^^^^^
 

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

 
 .. option:: StrictMode
 
    When `false` (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.
+
+.. option:: IgnoreVirtual
+
+   Determines whether virtual method parameters should be inspected.
+   Set to `true` to ignore them. Default is `false`.

clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters-virtual.cpp

+// RUN: %check_clang_tidy %s misc-unused-parameters %t -- \
+// RUN:   -config="{CheckOptions: [{key: misc-unused-parameters.IgnoreVirtual, value: true}]}" --
+
+struct Base {
+  int f(int foo) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: parameter 'foo' is unused [misc-unused-parameters]
+  // CHECK-FIXES: int f(int  /*foo*/) {
+    return 5;
+  }
+
+  virtual int f2(int foo) {
+    return 5;
+  }
+};
+
+struct Derived : Base {
+  int f2(int foo) override {
+    return 5;
+  }
+};