[clang-tidy] initial version of readability-convert-member-functions-to-static

mgehre · mgehre · commit ffca322266fc · 2019-07-16T21:19:00.000Z
Summary: Finds non-static member functions that can be made ``static``. I have run this check (repeatedly) over llvm-project. It made 1708 member functions ``static``. Out of those, I had to exclude 22 via ``NOLINT`` because their address was taken and stored in a variable of pointer-to-member type (e.g. passed to llvm::StringSwitch). It also made 243 member functions ``const``. (This is currently very conservative to have no false-positives and can hopefully be extended in the future.) You can find the results here: https://github.com/mgehre/llvm-project/commits/static_const_eval Reviewers: alexfh, aaron.ballman Subscribers: mgorny, xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D61749 llvm-svn: 366265
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -5,6 +5,7 @@ add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerSizeEmptyCheck.cpp
+  ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
@@ -0,0 +1,172 @@
+//===--- ConvertMemberFunctionsToStatic.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 "ConvertMemberFunctionsToStatic.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/SourceLocation.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+
+AST_MATCHER(CXXMethodDecl, hasTrivialBody) { return Node.hasTrivialBody(); }
+
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  return Node.isOverloadedOperator();
+}
+
+AST_MATCHER(CXXRecordDecl, hasAnyDependentBases) {
+  return Node.hasAnyDependentBases();
+}
+
+AST_MATCHER(CXXMethodDecl, isTemplate) {
+  return Node.getTemplatedKind() != FunctionDecl::TK_NonTemplate;
+}
+
+AST_MATCHER(CXXMethodDecl, isDependentContext) {
+  return Node.isDependentContext();
+}
+
+AST_MATCHER(CXXMethodDecl, isInsideMacroDefinition) {
+  const ASTContext &Ctxt = Finder->getASTContext();
+  return clang::Lexer::makeFileCharRange(
+             clang::CharSourceRange::getCharRange(
+                 Node.getTypeSourceInfo()->getTypeLoc().getSourceRange()),
+             Ctxt.getSourceManager(), Ctxt.getLangOpts())
+      .isInvalid();
+}
+
+AST_MATCHER_P(CXXMethodDecl, hasCanonicalDecl,
+              ast_matchers::internal::Matcher<CXXMethodDecl>, InnerMatcher) {
+  return InnerMatcher.matches(*Node.getCanonicalDecl(), Finder, Builder);
+}
+
+AST_MATCHER(CXXMethodDecl, usesThis) {
+  class FindUsageOfThis : public RecursiveASTVisitor<FindUsageOfThis> {
+  public:
+    bool Used = false;
+
+    bool VisitCXXThisExpr(const CXXThisExpr *E) {
+      Used = true;
+      return false; // Stop traversal.
+    }
+  } UsageOfThis;
+
+  // TraverseStmt does not modify its argument.
+  UsageOfThis.TraverseStmt(const_cast<Stmt *>(Node.getBody()));
+
+  return UsageOfThis.Used;
+}
+
+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.
+                      ),
+              isInsideMacroDefinition(),
+              hasCanonicalDecl(isInsideMacroDefinition()), usesThis())))
+          .bind("x"),
+      this);
+}
+
+/// \brief Obtain the original source code text from a SourceRange.
+static StringRef getStringFromRange(SourceManager &SourceMgr,
+                                    const LangOptions &LangOpts,
+                                    SourceRange Range) {
+  if (SourceMgr.getFileID(Range.getBegin()) !=
+      SourceMgr.getFileID(Range.getEnd()))
+    return {};
+
+  return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr,
+                              LangOpts);
+}
+
+static SourceRange getLocationOfConst(const TypeSourceInfo *TSI,
+                                      SourceManager &SourceMgr,
+                                      const LangOptions &LangOpts) {
+  assert(TSI);
+  const auto FTL = TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
+  assert(FTL);
+
+  SourceRange Range{FTL.getRParenLoc().getLocWithOffset(1),
+                    FTL.getLocalRangeEnd()};
+  // Inside Range, there might be other keywords and trailing return types.
+  // Find the exact position of "const".
+  StringRef Text = getStringFromRange(SourceMgr, LangOpts, Range);
+  size_t Offset = Text.find("const");
+  if (Offset == StringRef::npos)
+    return {};
+
+  SourceLocation Start = Range.getBegin().getLocWithOffset(Offset);
+  return {Start, Start.getLocWithOffset(strlen("const") - 1)};
+}
+
+void ConvertMemberFunctionsToStatic::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Definition = Result.Nodes.getNodeAs<CXXMethodDecl>("x");
+
+  // TODO: For out-of-line declarations, don't modify the source if the header
+  // is excluded by the -header-filter option.
+  DiagnosticBuilder Diag =
+      diag(Definition->getLocation(), "method %0 can be made static")
+      << Definition;
+
+  // TODO: Would need to remove those in a fix-it.
+  if (Definition->getMethodQualifiers().hasVolatile() ||
+      Definition->getMethodQualifiers().hasRestrict() ||
+      Definition->getRefQualifier() != RQ_None)
+    return;
+
+  const CXXMethodDecl *Declaration = Definition->getCanonicalDecl();
+
+  if (Definition->isConst()) {
+    // Make sure that we either remove 'const' on both declaration and
+    // definition or emit no fix-it at all.
+    SourceRange DefConst = getLocationOfConst(Definition->getTypeSourceInfo(),
+                                              *Result.SourceManager,
+                                              Result.Context->getLangOpts());
+
+    if (DefConst.isInvalid())
+      return;
+
+    if (Declaration != Definition) {
+      SourceRange DeclConst = getLocationOfConst(
+          Declaration->getTypeSourceInfo(), *Result.SourceManager,
+          Result.Context->getLangOpts());
+
+      if (DeclConst.isInvalid())
+        return;
+      Diag << FixItHint::CreateRemoval(DeclConst);
+    }
+
+    // Remove existing 'const' from both declaration and definition.
+    Diag << FixItHint::CreateRemoval(DefConst);
+  }
+  Diag << FixItHint::CreateInsertion(Declaration->getBeginLoc(), "static ");
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.h b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.h
@@ -0,0 +1,37 @@
+//===--- ConvertMemberFunctionsToStatic.h - clang-tidy ----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONVERTMEMFUNCTOSTATIC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONVERTMEMFUNCTOSTATIC_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONVERTMEMFUNCTOSTATIC_H
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -13,6 +13,7 @@
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerSizeEmptyCheck.h"
+#include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
@@ -57,6 +58,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-const-return-type");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
+    CheckFactories.registerCheck<ConvertMemberFunctionsToStatic>(
+        "readability-convert-member-functions-to-static");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -230,6 +230,11 @@ Improvements to clang-tidy
   If set to true, the check will provide fix-its with literal initializers
   (``int i = 0;``) instead of curly braces (``int i{};``).
 
+- New :doc:`readability-convert-member-functions-to-static
+  <clang-tidy/checks/readability-convert-member-functions-to-static>` check.
+
+  Finds non-static member functions that can be made ``static``.
+
 Improvements to include-fixer
 -----------------------------
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -257,6 +257,7 @@ Clang-Tidy Checks
    readability-braces-around-statements
    readability-const-return-type
    readability-container-size-empty
+   readability-convert-member-functions-to-static
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-convert-member-functions-to-static.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-convert-member-functions-to-static.rst
@@ -0,0 +1,14 @@
+.. 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, runnnig 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` to replace calls like
+``Instance.method()`` by ``Class::method()``.
diff --git a/clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp b/clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
