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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.



================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:225
+  <clang-tidy/checks/misc/unused-parameters>` check with new `IgnoreVirtual`
+  option to optionally ignore virtual methods (default `false`).
+
----------------
Don't really need to specify the default here, that's what the check documentation is for


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:309-310
 - 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 
+  <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.
----------------
Please revert this change, it's unrelated.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters-virtual.cpp:7
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: parameter 'foo' is unused [misc-unused-parameters]
+  // CHECK-FIXES: {{^  }}int f(int  /*foo*/) {{{$}}
+    return 5;
----------------
The Regex line start and end markers aren't necessary


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters-virtual.cpp:17
+struct Derived : Class {
+  int f2(int foo) {
+    return 5;
----------------
Small nit: Insert `override` here so it's obvious to readers of the test that this is a virtual function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147918/new/

https://reviews.llvm.org/D147918