[clang-tidy] Add option to ignore CRTP overrides in convert-member-functions-to-static

[njames93]

Summary

In cases where a member function is a CRTP override, marking the 'derived' as static is likely to reduce in less readable code.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability/convert-member-functions-to-static.rst

23

Please highlight Derived::shouldTraverse with double back-ticks.

[njames93]

Fix documentation.

clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.h

 /// This check finds C++ class methods than can be made static
 /// because they don't use the 'this' pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/
 /// readability-convert-member-functions-to-static.html
 class ConvertMemberFunctionsToStatic : public ClangTidyCheck {
 public:
-  ConvertMemberFunctionsToStatic(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  ConvertMemberFunctionsToStatic(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+
+private:
+  const bool IgnorePseudoOverrides;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONVERTMEMFUNCTOSTATIC_H

clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp

AST_MATCHER(CXXMethodDecl, usesThis) {
   } UsageOfThis;
 
   // TraverseStmt does not modify its argument.
   UsageOfThis.TraverseStmt(const_cast<Stmt *>(Node.getBody()));
 
   return UsageOfThis.Used;
 }
 
+AST_MATCHER(CXXMethodDecl, potentialCTRPOverride) {
+  for (const CXXBaseSpecifier &Base : Node.getParent()->bases()) {
+    if (const auto *RD = Base.getType()->getAsCXXRecordDecl())
+      if (RD->hasMemberName(Node.getDeclName()))
+        return true;
+  }
+  return false;
+}
+
 void ConvertMemberFunctionsToStatic::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       cxxMethodDecl(
           isDefinition(), isUserProvided(),
           unless(anyOf(
               isExpansionInSystemHeader(), isVirtual(), isStatic(),
               hasTrivialBody(), isOverloadedOperator(), cxxConstructorDecl(),
               cxxDestructorDecl(), cxxConversionDecl(), isTemplate(),
               isDependentContext(),
               ofClass(anyOf(
                   isLambda(),
                   hasAnyDependentBases()) // Method might become virtual
                                           // depending on template base class.
                       ),
+              IgnorePseudoOverrides ? potentialCTRPOverride()
+                                    : unless(anything()),
               isInsideMacroDefinition(),
               hasCanonicalDecl(isInsideMacroDefinition()), usesThis())))
           .bind("x"),
       this);
 }
 
 /// Obtain the original source code text from a SourceRange.
 static StringRef getStringFromRange(SourceManager &SourceMgr,
if (Definition->isConst()) {
     }
 
     // Remove existing 'const' from both declaration and definition.
     Diag << FixItHint::CreateRemoval(DefConst);
   }
   Diag << FixItHint::CreateInsertion(Declaration->getBeginLoc(), "static ");
 }
 
+ConvertMemberFunctionsToStatic::ConvertMemberFunctionsToStatic(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnorePseudoOverrides(Options.get("IgnorePseudoOverrides", false)) {}
+
+void ConvertMemberFunctionsToStatic::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnorePseudoOverrides", IgnorePseudoOverrides);
+}
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Improved `modernize-use-emplace <clang-tidy/checks/modernize/use-emplace.html>`_ check.
   ``push_front`` on STL-style containers and replacing them with ``emplace``
   or ``emplace_front``.
 
 - Improved `modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default.html>`_ check.
 
   The check now skips unions since in this case a default constructor with empty body
   is not equivalent to the explicitly defaulted one.
 
+- Improved :doc:`readability-convert-member-functions-to-static
+  <clang-tidy/checks/readability/convert-member-functions-to-static>` check. 
+
+  This check can now ignore CRTP pseudooverrides.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/readability/convert-member-functions-to-static.rst

 .. title:: clang-tidy - readability-convert-member-functions-to-static
 
 readability-convert-member-functions-to-static
 ==============================================
 
 Finds non-static member functions that can be made ``static``
 because the functions don't use ``this``.
 
 After applying modifications as suggested by the check, running the check again
 might find more opportunities to mark member functions ``static``.
 
 After making a member function ``static``, you might want to run the check
 `readability-static-accessed-through-instance <../readability/static-accessed-through-instance.html>`_ to replace calls like
 ``Instance.method()`` by ``Class::method()``.
+
+Options
+-------
+
+.. option:: IgnorePseudoOverrides
+
+  When `true`, if there is a base class with a method with the same name, no
+  warning will be emitted. Often occurs with CRTP like classes.
+  For the case below a warning would only be emitted for 
+  ``Derived::shouldTraverse`` if this is set to `false`.
+  
+  Default value is `false`
+
+  .. code-block:: c++
+
+    template <typename T>
+    struct CRTPBase {
+      bool shouldTraverse();
+    };
+
+    struct Derived : CRTPBase<Derived> {
+      bool shouldTraverse() { return false; }
+    };

clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-crtp.cpp

+// If the option is true, expect no warnigs, however the check_clang_tidy script can't handle that case.
+// RUN: clang-tidy %s -checks=-*,readability-convert-member-functions-to-static \
+// RUN: -config='{CheckOptions: {readability-convert-member-functions-to-static.IgnorePseudoOverrides: true}}' \
+// RUN: -- -std=c++14 | count 0
+
+// RUN: %check_clang_tidy %s readability-convert-member-functions-to-static %t -- \
+// RUN: -config='{CheckOptions: {readability-convert-member-functions-to-static.IgnorePseudoOverrides: false}}' 
+template<typename T>
+class CRTP{
+  bool foo();
+  bool bar();
+};
+
+class CRTPInstance : public CRTP<CRTPInstance> {
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: method 'foo' can be made static
+  bool foo() { return false; }
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: method 'bar' can be made static
+  void bar() { return; }
+};