mgehre created this revision. mgehre added reviewers: alexfh, aaron.ballman. Herald added subscribers: xazax.hun, mgorny. Herald added a project: clang.
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 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D61749 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.cpp clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/docs/clang-tidy/checks/readability-static-const-method.rst clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp
Index: clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/readability-static-const-method.cpp @@ -0,0 +1,317 @@ +// RUN: %check_clang_tidy %s readability-static-const-method %t + +class DoNotMakeEmptyStatic { + void emptyMethod() {} + void empty_method_out_of_line(); +}; + +void DoNotMakeEmptyStatic::empty_method_out_of_line() {} + +struct Str { + void const_method() const; + void non_const_method(); +}; + +class A; + +void const_use(const A *); +void non_const_use(A *); +void const_use(const A &); +void non_const_use(A &); + +class A { + int field; + const int const_field; + static int static_field; + Str S; + Str Sref; + Str *Sptr; + + 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() { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const + // CHECK-FIXES: {{^}} int read_field() const { + return field; + } + + int read_const_field() { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const + // CHECK-FIXES: {{^}} int read_const_field() const { + return const_field; + } + + void write_field() { + field = 1; + } + + int call_non_const_member() { return read_field(); } + + int call_const_member() { + // TODO: :[[@LINE-1]]:7: warning: method 'call_const_member' can be made const + // TODO: {{^}} int call_const_member() const { + return already_const(); + } + + 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(); + } + + void call_non_const_member_on_field() { S.non_const_method(); } + + void call_const_member_on_field() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + S.const_method(); + } + + void call_non_const_member_on_pointee() { + Sptr->non_const_method(); + } + + void call_const_member_on_pointee() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + Sptr->const_method(); + } + + Str *return_pointer() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + return Sptr; + } + + void call_non_const_member_on_ref() { + Sref.non_const_method(); + } + + void call_const_member_on_ref() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + Sref.const_method(); + } + + void call_const_member_on_ref_of_ref() { + const auto &L = Sref; + // Technically, this method could be const-qualified, + // but it might not be logically const. + L.const_method(); + } + + Str &return_ref() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + return Sref; + } + + 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; + } + + void const_use() { + // TODO: :[[@LINE-1]]:8: warning: method 'const_use' can be made const + ::const_use(this); + } + void non_const_use() { ::non_const_use(this); } + + void const_use_ref() { + // TODO: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const + ::const_use(*this); + } + void non_const_use_ref() { ::non_const_use(*this); } + + static int out_of_line_already_static(); + + void out_of_line_call_const(); + // TODO: {{^}} void out_of_line_call_const() const; + 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_const() { + // TODO: :[[@LINE-1]]:9: warning: method 'out_of_line_call_const' can be made const + // TODO: {{^}}void A::out_of_line_call_const() const { + int i = already_const(); +} + +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 KeepConstCast { + const int *f() const; + int *f() { // Needs to stay non-const. + return const_cast<int *>(static_cast<const KeepConstCast*>(this)->f()); + } +}; + +struct KeepVirtual { + int i; + virtual int f() { return 0; } + virtual int g() { return i; } + virtual int h() const { return 0; } +}; + +struct KeepVirtualDerived : public KeepVirtual { + int f() { return 0; } + int g() override { 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; } + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'operator+' can be made const + // CHECK-FIXES: {{^}} int operator+() const { + operator int() { return 0; } + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'operator int' can be made const + // CHECK-FIXES: {{^}} operator int() const { +}; + +void KeepLambdas() { + auto F = +[]() { return 0; }; + auto F2 = []() { return 0; }; +} + +template <class Base> +struct KeepWithTemplateBase : public Base { + int i; + // We cannot make these methods static or const because they might need to override + // a function from Base. + int static_f() { return 0; } + int const_f() { return i; } +}; + +template <class T> +struct KeepTemplateClass { + int i; + // We cannot make these methods static or const because a specialization + // might use *this differently. + int static_f() { return 0; } + int const_f() { return i; } +}; + +struct KeepTemplateMethod { + int i; + // We cannot make these methods static or const because a specialization + // might use *this differently. + template <class T> + static int static_f() { return 0; } + + template <class T> + int const_f() { return i; } +}; + +void instantiate() { + struct S {}; + KeepWithTemplateBase<S> I1; + I1.static_f(); + I1.const_f(); + + KeepTemplateClass<int> I2; + I2.static_f(); + I2.const_f(); + + KeepTemplateMethod I3; + I3.static_f<int>(); + I3.const_f<int>(); +} + +struct TrailingReturn { + int i; + auto f() -> int { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'f' can be made const + // CHECK-FIXES: {{^}} auto f() const -> int { + return i; + } + auto g() const -> int { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'g' can be made static + // CHECK-FIXES: {{^}} static auto g() -> int { + return 0; + } +}; + +struct NoFixitInMacro { +#define CONST const + int no_use_macro() CONST { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'no_use_macro' can be made static + // CHECK-FIXES: {{^}} int no_use_macro() CONST { + return 0; + } + +#define ADD_CONST(F) F const + int ADD_CONST(no_use_macro2()) { + return 0; + } + +#define FUN const_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) +}; + +// Real-world code, see clang::ObjCInterfaceDecl. +class DataPattern { + int& data() const; +public: + + int& get() { + return data(); + } + + const int& get() const { + return const_cast<DataPattern *>(this)->get(); + } + + void set() { + data() = 42; + } +}; \ No newline at end of file Index: clang-tools-extra/docs/clang-tidy/checks/readability-static-const-method.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/readability-static-const-method.rst @@ -0,0 +1,19 @@ +.. title:: clang-tidy - readability-static-const-method + +readability-static-const-method +=============================== + +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. + +After applying modifications as suggested by the check, runnnig the check again +might find more opportunities to mark member functions ``const`` or ``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()``. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -261,6 +261,7 @@ readability-simplify-boolean-expr readability-simplify-subscript-expr readability-static-accessed-through-instance + readability-static-const-method readability-static-definition-in-anonymous-namespace readability-string-compare readability-uniqueptr-delete-release Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -69,6 +69,11 @@ - ... +- New :doc:`readability-static-const-method + <clang-tidy/checks/readability-static-const-method>` check. + + Finds non-static member functions that can be made ``const`` or ``static``. + Improvements to include-fixer ----------------------------- Index: clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.h @@ -0,0 +1,36 @@ +//===--- StaticConstMethodCheck.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_STATICCONSTMETHODCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATICCONSTMETHODCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// This check finds C++ class methods than can be made static or const, +/// by looking at the usage of the *this pointer within the method. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-const-method.html +class StaticConstMethodCheck : public ClangTidyCheck { +public: + StaticConstMethodCheck(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_STATICCONSTMETHODCHECK_H Index: clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/readability/StaticConstMethodCheck.cpp @@ -0,0 +1,241 @@ +//===--- StaticConstMethodCheck.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 "StaticConstMethodCheck.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 { + +enum UsageKind { Unused, Const, NonConst }; + +class FindUsageOfThis : public RecursiveASTVisitor<FindUsageOfThis> { +public: + FindUsageOfThis(ASTContext &Ctxt) : Ctxt(Ctxt) {} + UsageKind Usage = Unused; + + ASTContext &Ctxt; + + template <class T> const T *getParent(const Expr *E) { + auto Parents = Ctxt.getParents(*E); + if (Parents.size() != 1) + return nullptr; + + return Parents.begin()->get<T>(); + } + + bool VisitUnresolvedMemberExpr(const UnresolvedMemberExpr *) { + // An UnresolvedMemberExpr might resolve to a non-const non-static + // member function. + Usage = NonConst; + return false; // Stop traversal. + } + + bool VisitCXXConstCastExpr(const CXXConstCastExpr *) { + // Workaround to support the pattern + // class C { + // const S *get() const; + // S* get() { + // return const_cast<S*>(const_cast<const C*>(this)->get()); + // } + // }; + // Here, we don't want to make the second 'get' const even though + // it only calls a const member function on this. + Usage = NonConst; + return false; // Stop traversal. + } + + bool VisitCXXThisExpr(const CXXThisExpr *E) { + Usage = Const; + + auto *Parent = getParent<Expr>(E); + + // Detect (recursive) MemberExpr casted to rvalue or const. + while (auto *Member = dyn_cast_or_null<MemberExpr>(Parent)) { + Parent = getParent<Expr>(Member); + + if (auto *Cast = dyn_cast_or_null<ImplicitCastExpr>(Parent)) { + // To preserve "logical" const, we don't allow to change the pointee of + // a member or call const methods (like unique_ptr::get). + if (Cast->getCastKind() == CK_LValueToRValue && + Cast->getType()->isBuiltinType()) { + return true; + } + + if (Cast->getCastKind() == CK_NoOp && + Cast->getType().isConstQualified() && + Cast->getType()->isBuiltinType()) + return true; + } + } + + Usage = NonConst; + return false; // Stop traversal. + } +}; + +void StaticConstMethodCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxMethodDecl(unless(isExpansionInSystemHeader()), isDefinition()) + .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 StringRef(); // Empty string. + } + + return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr, + LangOpts); +} + +static SourceRange getLocationOfConst(const TypeSourceInfo *TSI, + SourceManager &SourceMgr, + const LangOptions &LangOpts) { + if (!TSI) + return {}; + FunctionTypeLoc FTL = + TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>(); + if (!FTL) + return {}; + + auto Range = SourceRange{FTL.getRParenLoc().getLocWithOffset(1), + FTL.getLocalRangeEnd()}; + // Inside Range, there might be other keywords and trailing return types. + // Find the exact position of "const". + auto Text = getStringFromRange(SourceMgr, LangOpts, Range); + auto Offset = Text.find("const"); + if (Offset == StringRef::npos) + return {}; + + auto Start = Range.getBegin().getLocWithOffset(Offset); + return {Start, Start.getLocWithOffset(strlen("const") - 1)}; +} + +static SourceLocation getConstInsertionPoint(const CXXMethodDecl *M) { + auto TSI = M->getTypeSourceInfo(); + if (!TSI) + return {}; + FunctionTypeLoc FTL = + TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>(); + if (!FTL) + return {}; + + return FTL.getRParenLoc().getLocWithOffset(1); +} + +// Returns `true` if `Range` is inside a macro definition. +static bool insideMacroDefinition(const MatchFinder::MatchResult &Result, + SourceRange Range) { + return !clang::Lexer::makeFileCharRange( + clang::CharSourceRange::getCharRange(Range), + *Result.SourceManager, Result.Context->getLangOpts()) + .isValid(); +} + +void StaticConstMethodCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Definition = Result.Nodes.getNodeAs<CXXMethodDecl>("x"); + if (!Definition->isUserProvided()) + return; + if (Definition->isStatic()) + return; // Nothing we can improve. + if (Definition->isVirtual()) + return; + if (Definition->getParent()->hasAnyDependentBases()) + return; // Method might become virtual depending on template base class. + if (Definition->hasTrivialBody()) + return; + if (Definition->isOverloadedOperator() && Definition->isConst()) + return; // Nothing we can improve. + if (Definition->getTemplateSpecializationKind() != TSK_Undeclared) + return; + if (Definition->getTemplatedKind() != FunctionDecl::TK_NonTemplate) + return; // We might not see all specializations. + if (isa<CXXConstructorDecl>(Definition) || isa<CXXDestructorDecl>(Definition)) + return; + if (Definition->isDependentContext()) + return; + if (Definition->getParent()->isLambda()) + return; + if (insideMacroDefinition( + Result, + Definition->getTypeSourceInfo()->getTypeLoc().getSourceRange())) + return; + + auto Declaration = Definition->getCanonicalDecl(); + + if (Declaration != Definition && + insideMacroDefinition( + Result, + Declaration->getTypeSourceInfo()->getTypeLoc().getSourceRange())) + return; + + FindUsageOfThis UsageOfThis(*Result.Context); + // TraverseStmt does not modify its argument. + UsageOfThis.TraverseStmt(const_cast<Stmt *>(Definition->getBody())); + + // TODO: For out-of-line declarations, don't modify the source if the header + // is excluded by the -header-filter option. + if (UsageOfThis.Usage == Unused && !Definition->isOverloadedOperator() && + !isa<CXXConversionDecl>(Definition)) { + auto Diag = diag(Definition->getLocation(), "method %0 can be made static") + << Definition; + + if (Definition->isConst()) { + // Make sure that we either remove 'const' on both declaration and + // definition or emit no fix-it at all. + auto DefConst = getLocationOfConst(Definition->getTypeSourceInfo(), + *Result.SourceManager, + Result.Context->getLangOpts()); + + if (DefConst.isInvalid()) + return; + + if (Declaration != Definition) { + auto 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 "); + + } else if (UsageOfThis.Usage <= Const && !Definition->isConst()) { + auto Diag = diag(Definition->getLocation(), "method %0 can be made const") + << Definition + << FixItHint::CreateInsertion( + getConstInsertionPoint(Definition), " const"); + if (Declaration != Definition) { + Diag << FixItHint::CreateInsertion(getConstInsertionPoint(Declaration), + " const"); + } + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -38,6 +38,7 @@ #include "SimplifyBooleanExprCheck.h" #include "SimplifySubscriptExprCheck.h" #include "StaticAccessedThroughInstanceCheck.h" +#include "StaticConstMethodCheck.h" #include "StaticDefinitionInAnonymousNamespaceCheck.h" #include "StringCompareCheck.h" #include "UniqueptrDeleteReleaseCheck.h" @@ -90,6 +91,8 @@ "readability-simplify-subscript-expr"); CheckFactories.registerCheck<StaticAccessedThroughInstanceCheck>( "readability-static-accessed-through-instance"); + CheckFactories.registerCheck<StaticConstMethodCheck>( + "readability-static-const-method"); CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>( "readability-static-definition-in-anonymous-namespace"); CheckFactories.registerCheck<StringCompareCheck>( Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -31,6 +31,7 @@ SimplifyBooleanExprCheck.cpp SimplifySubscriptExprCheck.cpp StaticAccessedThroughInstanceCheck.cpp + StaticConstMethodCheck.cpp StaticDefinitionInAnonymousNamespaceCheck.cpp StringCompareCheck.cpp UniqueptrDeleteReleaseCheck.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits