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

[mgehre]

Summary

Finds non-static member functions that can be made const or static.
The check conservatively tries to preserve logical costness in favor of
physical costness. It will not recommend to make member functions const
that call (const) member functions on a non-static data members,
e.g. std::unique_ptr::get because that might indicate logical
non-costness. Currently, it will only make member functions const if they
only access to this is to read members of builtin type.

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

[Eugene.Zelenko]

@JonasToth tried to implement const check, so probably const and static part should be split. It's hard to tell about state of existing implementation, but you could continue it or at least borrow ideas and tests.

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

33 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

101 ↗

(On Diff #198882)

Please elide braces.

103 ↗

(On Diff #198882)

You could return {}

124 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

129 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

134 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

183 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

199 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

213 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

226 ↗

(On Diff #198882)

Unnecessary empty line.

228 ↗

(On Diff #198882)

Please don't use auto when type is not spelled explicitly in same statement or it's iterator.

clang-tools-extra/docs/ReleaseNotes.rst

72 ↗

(On Diff #198882)

Please rebase from trunk.

[lebedev.ri]

In D61749#1497162, @Eugene.Zelenko wrote:

@JonasToth tried to implement const check, so probably const and static part should be split.

I agree, there is D54943, which should be 99% ready to go, and is only stuck due to @JonasToth
time constraints, IIRC. Treating *this pointer should simply be a logical extension of
that check. (unless it already does that, or there is some other patch i forgot about)

I.e. let's split this, and start with static part.

Also, you are aware of ExprMutationAnalyzer?

It's hard to tell about state of existing implementation, but you could continue it or at least borrow ideas and tests.

[mgehre]

I have thought about splitting the check into separate static & const, but kept it together because
the analysis for both cases is very similar, i.e. finding the usage of this.

Also, both checks share many preconditions, e.g. not applying them to virtual methods or methods
on class with a template base class, not applying them to constructor, not applying them to operators, etc
(and those preconditons are not shared with D54943).

I had a look at D54943. Nice change! It will be very valuable to have the const analysis for variables!
Note, though, that the hard part of of making methods
const is not the detailed usage analysis as its done by the ExprMutationAnalyzer. Instead, the hard part
is to avoid false-positives that make the check unusable.
I had a version of this check that made a very detailed analysis just like ExprMutationAnalyzer
(restricted to this). But then most of the functions it flagged as const
(technically correct) on the LLVM code base we would not want to make const (logically). Examples:

class S {
  // Technically const (the pointer Pimp itself is not modified), but logically should not be const
  //  because we want to consider *Pimp as part of the data.
  void setFlag() {
    *Pimp = 4;
  }

  int* Pimp;
};

class S {
  void modify() {
     // Technically const because we call a const method
    // "pointer unique_ptr::operator->() const;"
    // but logically not const.
    U->modify();
  }

  std::unique_ptr<Object> U;
};

// Real-world code, see clang::ObjCInterfaceDecl.
class DataPattern {
  int& data() const;
public:

  int& get() { // Must not be const, even though it only calls const methods
    return data();
  }

  const int& get() const {
    return const_cast<DataPattern *>(this)->get();
  }

  void set() { // Must not be const, even though it only calls const methods
    data() = 42;
  }
};

and many more.

So the hard thing for the const check was to actually remove the sophisticated analysis I had, and come up with
something much simpler that would have no false positive (otherwise users will just disable the check) while
still finding something. I fear that the same could happen when we apply D54943 to this directly. It might
be easier to keep those two cases separate.

[lebedev.ri]

In D61749#1498541, @mgehre wrote:

...

Still, this is two separate checks, logically.
You can put the FindUsageOfThis e.g. into a new file in clang-tidy/utils.

[JonasToth]

Hey, I do like your check!

I think it would be great to implement this analysis in utils/ as roman suggested. Then we have bigger flexibility.
The ExprMutAnalyzer was requested to be moved to clang/Analysis/ as the CSA folks wanted to reuse it at some point. That might be the case with your logic as well, as it is still related to ExprMutAnalyzer even though its not the same.

Another issue why the clang-tidy part for the variable const-suggestions doesn't move forward is that ExprMutAnalyzer itself is blocked due to @shuaiwang not having time as well. We have a view nasty cases that produce false positives and especially pointer analysis is complicated.

For that I think that @mgehre approach of producing something useful now should not be blocked on us. Especially since its unclear when we can continue our work substantially. Preparing for joining forces later would be great though! I think this is possible with having just a class/set of functions for the analysis that can be used in different parts of clang tools is sufficient for that.

[JonasToth]

clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp

209 ↗

(On Diff #198882)

Does it pass here?
I looks a bit weird, shouldnt the lambda be called for this to work (this is the unary + right?)?

312 ↗

(On Diff #198882)

I think more template tests wouldn't hurt. From the other checks experience I can safely say we got burned a few times :)
Instantiating the templates with builtin arrays, pointers, references and different qualifiers usually produces interesting test cases as well, that need to be handled properly.

Another thing that comes to mind with templates are overloaded operators.

template <class Foo>
void bar() {
     Foo x1, x2;
     Foo y = x1 + x2;
}

Builtins are not changed by operator+ but that can not be said about other types in general (maybe with concepts used properly).

[mgehre]

clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp

209 ↗

(On Diff #198882)

https://stackoverflow.com/questions/18889028/a-positive-lambda-what-sorcery-is-this

312 ↗

(On Diff #198882)

The check only checks templates instantiations (so we will see no template parameters, just ordinary types). The plus here will be be function call in the AST of the instantiation when Foo has an overloaded operator+.
The current version will never propose to make bar() const when a method on this is called.
I can add the test to show this.

[JonasToth]

clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp

209 ↗

(On Diff #198882)

Thank you C++ for showing my simple mindedness all the time :D
Its fine then and I will keep it in my mind for future test cases I check against ;)

312 ↗

(On Diff #198882)

Yes please add a test to show they are not analyzed.

[mgehre]

clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp

312 ↗

(On Diff #198882)

I will now restrict this PR to the static part of the check, and I fail to come up with an example where operators could make a static looking function non-static.

[mgehre]

Remove `const` part from this PR. Address comments.

[mgehre]

Ping

[mgehre]

Friendly ping. Are there outstanding concerns that I could address?

[aaron.ballman]

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

96 ↗

(On Diff #199329)

I'm not super keen on the check name. It's not very descriptive -- does this operate on static methods, does it make methods static, does it turn global dynamic initialization into static method initialization?

How about: readability-convert-functions-to-static or something more descriptive of the functionality?

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

25 ↗

(On Diff #199329)

Do you need this constructor at all? If so, = default; it.

28–31 ↗

(On Diff #199329)

Does this find a usage of this in unevaluated contexts? e.g.,

struct S {
  size_t derp() { return sizeof(this); }
};

I am hoping the answer is yes, because we wouldn't want to convert that into a static function. Probably worth a test.

56–57 ↗

(On Diff #199329)

Under what circumstances can this happen?

58 ↗

(On Diff #199329)

Can use auto here.

63 ↗

(On Diff #199329)

SourceRange Range{...};

68 ↗

(On Diff #199329)

Don't use auto here.

87–108 ↗

(On Diff #199329)

Much of this logic should be hoisted into the AST matcher code rather than handled one-off here. Some of it can be done directly, others may require new local matchers to be defined.

116 ↗

(On Diff #199329)

Formatting is wrong here -- run the patch through clang-format.

136–149 ↗

(On Diff #199329)

const isn't the only thing to worry about though, no? You need to handle things like:

struct S {
  void f() volatile;
  void g() &;
  void h() &&;
};

Another one I am curious about are computed noexcept specifications and whether those are handled properly. e.g.,

struct S {
  int i;
  void foo(SomeClass C) noexcept(noexcept(C + i));
};

clang-tools-extra/test/clang-tidy/readability-static-method.cpp

99 ↗

(On Diff #199329)

This is too subtle for my tastes. Another way to trigger the conversion is a type cast, which is far more understandable to anyone reading the code a few years from now.

[mgehre]

• Address review comments

[mgehre]

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

96 ↗

(On Diff #199329)

I agree that we should find a better name. With readability-convert-functions-to-static, I miss the difference between adding static linkage to a free function versus making a member function static.
How about readability-convert-member-functions-to-static or readability-convert-method-to-static?

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

25 ↗

(On Diff #199329)

Removed

28–31 ↗

(On Diff #199329)

I added a test

56–57 ↗

(On Diff #199329)

I will check

136–149 ↗

(On Diff #199329)

I added tests for those cases and disabled fix-it generation for them to keep the code simple (for now).

[mgehre]

Move checks into AST matchers

[aaron.ballman]

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

96 ↗

(On Diff #199329)

I think readability-convert-member-functions-to-static would be a good name.

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

35–45 ↗

(On Diff #208785)

Is there a reason we can't use cxxConstructorDecl(), cxxDestructorDecl(), and cxxConversionDecl() instead?

130 ↗

(On Diff #208785)

What does this do for code like:

#define constantly_annoying volatile

struct S {
  void func() constantly_annoying {}
};

136–149 ↗

(On Diff #199329)

There's an obscure edge case missing:

struct S {
  void f() __restrict;
};

[lebedev.ri]

Sorry, it does not appear that i'm being useful in this review.

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

89–101 ↗

(On Diff #208785)

I'm pretty sure you can do

unless(anyOf(isExpansionInSystemHeader(), isVirtual(), ...))

clang-tools-extra/test/clang-tidy/readability-static-method.cpp

103 ↗

(On Diff #208785)

Is there a test where a lambda (variant with and without capturing this, either explicitly, or implicitly)
is within a non-static class function?

[mgehre]

Implement comments

[mgehre]

This should address all remaining comments.

[aaron.ballman]

LGTM aside from a tiny nit.

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

110 ↗

(On Diff #209360)

const auto *

[mgehre]

Thank you for the review, Aaron!

clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerSizeEmptyCheck.cpp
+  ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp

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

+//===--- 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

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

+//===--- 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

clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerSizeEmptyCheck.h"
+#include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "IsolateDeclarationCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ConstReturnTypeCheck>(
         "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>(
         "readability-deleted-default");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- The :doc:`misc-throw-by-value-catch-by-reference
   object caught by value.
 
 - Added `UseAssignment` option to :doc:`cppcoreguidelines-pro-type-member-init
   <clang-tidy/checks/cppcoreguidelines-pro-type-member-init>`
 
   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
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    portability-simd-intrinsics
    readability-avoid-const-params-in-decls
    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
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name
    readability-isolate-declaration

clang-tools-extra/trunk/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, 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()``.

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

+// RUN: %check_clang_tidy %s readability-convert-member-functions-to-static %t
+
+class DoNotMakeEmptyStatic {
+  void emptyMethod() {}
+  void empty_method_out_of_line();
+};
+
+void DoNotMakeEmptyStatic::empty_method_out_of_line() {}
+
+class A {
+  int field;
+  const int const_field;
+  static int static_field;
+
+  void no_use() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'no_use' can be made static
+    // CHECK-FIXES: {{^}}  static void no_use() {
+    int i = 1;
+  }
+
+  int read_field() {
+    return field;
+  }
+
+  void write_field() {
+    field = 1;
+  }
+
+  int call_non_const_member() { return read_field(); }
+
+  int call_static_member() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'call_static_member' can be made static
+    // CHECK-FIXES: {{^}}  static int call_static_member() {
+    already_static();
+  }
+
+  int read_static() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_static' can be made static
+    // CHECK-FIXES: {{^}}  static int read_static() {
+    return static_field;
+  }
+  void write_static() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'write_static' can be made static
+    // CHECK-FIXES: {{^}}  static void write_static() {
+    static_field = 1;
+  }
+
+  static int already_static() { return static_field; }
+
+  int already_const() const { return field; }
+
+  int already_const_convert_to_static() const {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'already_const_convert_to_static' can be made static
+    // CHECK-FIXES: {{^}}  static int already_const_convert_to_static() {
+    return static_field;
+  }
+
+  static int out_of_line_already_static();
+
+  void out_of_line_call_static();
+  // CHECK-FIXES: {{^}}  static void out_of_line_call_static();
+  int out_of_line_const_to_static() const;
+  // CHECK-FIXES: {{^}}  static int out_of_line_const_to_static() ;
+};
+
+int A::out_of_line_already_static() { return 0; }
+
+void A::out_of_line_call_static() {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: method 'out_of_line_call_static' can be made static
+  // CHECK-FIXES: {{^}}void A::out_of_line_call_static() {
+  already_static();
+}
+
+int A::out_of_line_const_to_static() const {
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'out_of_line_const_to_static' can be made static
+  // CHECK-FIXES: {{^}}int A::out_of_line_const_to_static() {
+  return 0;
+}
+
+struct KeepVirtual {
+  virtual int f() { return 0; }
+  virtual int h() const { return 0; }
+};
+
+struct KeepVirtualDerived : public KeepVirtual {
+  int f() { return 0; }
+  int h() const override { return 0; }
+};
+
+// Don't add 'static' to special member functions and operators.
+struct KeepSpecial {
+  KeepSpecial() { int L = 0; }
+  ~KeepSpecial() { int L = 0; }
+  int operator+() { return 0; }
+  operator int() { return 0; }
+};
+
+void KeepLambdas() {
+  using FT = int (*)();
+  auto F = static_cast<FT>([]() { return 0; });
+  auto F2 = []() { return 0; };
+}
+
+template <class Base>
+struct KeepWithTemplateBase : public Base {
+  int i;
+  // We cannot make these methods static because they might need to override
+  // a function from Base.
+  int static_f() { return 0; }
+};
+
+template <class T>
+struct KeepTemplateClass {
+  int i;
+  // We cannot make these methods static because a specialization
+  // might use *this differently.
+  int static_f() { return 0; }
+};
+
+struct KeepTemplateMethod {
+  int i;
+  // We cannot make these methods static because a specialization
+  // might use *this differently.
+  template <class T>
+  static int static_f() { return 0; }
+};
+
+void instantiate() {
+  struct S {};
+  KeepWithTemplateBase<S> I1;
+  I1.static_f();
+
+  KeepTemplateClass<int> I2;
+  I2.static_f();
+
+  KeepTemplateMethod I3;
+  I3.static_f<int>();
+}
+
+struct Trailing {
+  auto g() const -> int {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'g' can be made static
+    // CHECK-FIXES: {{^}}  static auto g() -> int {
+    return 0;
+  }
+
+  void vol() volatile {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'vol' can be made static
+    return;
+  }
+
+  void ref() const & {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'ref' can be made static
+    return;
+  }
+  void refref() const && {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'refref' can be made static
+    return;
+  }
+
+  void restr() __restrict {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'restr' can be made static
+    return;
+  }
+};
+
+struct UnevaluatedContext {
+  void f() { sizeof(this); }
+
+  void noex() noexcept(noexcept(this));
+};
+
+struct LambdaCapturesThis {
+  int Field;
+
+  int explicitCapture() {
+    return [this]() { return Field; }();
+  }
+
+  int implicitCapture() {
+    return [&]() { return Field; }();
+  }
+};
+
+struct NoFixitInMacro {
+#define CONST const
+  int no_use_macro_const() CONST {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'no_use_macro_const' can be made static
+    return 0;
+  }
+
+#define ADD_CONST(F) F const
+  int ADD_CONST(no_use_macro2()) {
+    return 0;
+  }
+
+#define FUN no_use_macro()
+  int i;
+  int FUN {
+    return i;
+  }
+
+#define T(FunctionName, Keyword) \
+  Keyword int FunctionName() { return 0; }
+#define EMPTY
+  T(A, EMPTY)
+  T(B, static)
+
+#define T2(FunctionName) \
+  int FunctionName() { return 0; }
+  T2(A2)
+
+#define VOLATILE volatile
+  void volatileMacro() VOLATILE {
+    // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'volatileMacro' can be made static
+    return;
+  }
+};