diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -5,6 +5,7 @@ BracesAroundStatementsCheck.cpp ConstReturnTypeCheck.cpp ContainerSizeEmptyCheck.cpp + ConvertMemberFunctionsToStatic.cpp DeleteNullPointerCheck.cpp DeletedDefaultCheck.cpp ElseAfterReturnCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.h b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.h new file mode 100644 --- /dev/null +++ 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/ConvertMemberFunctionsToStatic.cpp b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp new file mode 100644 --- /dev/null +++ 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, InnerMatcher) { + return InnerMatcher.matches(*Node.getCanonicalDecl(), Finder, Builder); +} + +AST_MATCHER(CXXMethodDecl, usesThis) { + class FindUsageOfThis : public RecursiveASTVisitor { + 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(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); + auto FTL = TSI->getTypeLoc().IgnoreParens().getAs(); + 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("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/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- 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 @@ "readability-const-return-type"); CheckFactories.registerCheck( "readability-container-size-empty"); + CheckFactories.registerCheck( + "readability-convert-member-functions-to-static"); CheckFactories.registerCheck( "readability-delete-null-pointer"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -201,6 +201,11 @@ 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 + ` 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 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -255,6 +255,7 @@ 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 new file mode 100644 --- /dev/null +++ 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp @@ -0,0 +1,218 @@ +// 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([]() { return 0; }); + auto F2 = []() { return 0; }; +} + +template +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 +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 + static int static_f() { return 0; } +}; + +void instantiate() { + struct S {}; + KeepWithTemplateBase I1; + I1.static_f(); + + KeepTemplateClass I2; + I2.static_f(); + + KeepTemplateMethod I3; + I3.static_f(); +} + +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; + } +};