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 @@ -15,6 +15,7 @@ InconsistentDeclarationParameterNameCheck.cpp IsolateDeclarationCheck.cpp MagicNumbersCheck.cpp + MakeMemberFunctionConstCheck.cpp MisleadingIndentationCheck.cpp MisplacedArrayIndexCheck.cpp NamedParameterCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.h b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.h @@ -0,0 +1,34 @@ +//===--- MakeMemberFunctionConstCheck.h - clang-tidy ------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAKEMEMBERFUNCTIONCONSTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MAKEMEMBERFUNCTIONCONSTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Finds non-static member functions that can be made 'const'. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html +class MakeMemberFunctionConstCheck : public ClangTidyCheck { +public: + MakeMemberFunctionConstCheck(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_MAKEMEMBERFUNCTIONCONSTCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp @@ -0,0 +1,264 @@ +//===--- MakeMemberFunctionConstCheck.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 "MakeMemberFunctionConstCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.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(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); +} + +enum UsageKind { Unused, Const, NonConst }; + +class FindUsageOfThis : public RecursiveASTVisitor { + ASTContext &Ctxt; + +public: + FindUsageOfThis(ASTContext &Ctxt) : Ctxt(Ctxt) {} + UsageKind Usage = Unused; + + template const T *getParent(const Expr *E) { + ASTContext::DynTypedNodeList Parents = Ctxt.getParents(*E); + if (Parents.size() != 1) + return nullptr; + + return Parents.begin()->get(); + } + + 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(const_cast(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. + } + + // Our AST is + // `-ImplicitCastExpr + // (possibly `-UnaryOperator Deref) + // `-CXXThisExpr 'S *' this + bool VisitUser(const ImplicitCastExpr *Cast) { + if (Cast->getCastKind() != CK_NoOp) + return false; // Stop traversal. + + // Only allow NoOp cast to 'const S' or 'const S *'. + QualType QT = Cast->getType(); + if (QT->isPointerType()) + QT = QT->getPointeeType(); + + if (!QT.isConstQualified()) + return false; // Stop traversal. + + const auto *Parent = getParent(Cast); + if (!Parent) + return false; // Stop traversal. + + if (isa(Parent)) + return true; // return (const S*)this; + + if (isa(Parent)) + return true; // use((const S*)this); + + // ((const S*)this)->Member + if (const auto *Member = dyn_cast(Parent)) + return VisitUser(Member, /*OnConstObject=*/true); + + return false; // Stop traversal. + } + + // If OnConstObject is true, then this is a MemberExpr using + // a constant this, i.e. 'const S' or 'const S *'. + bool VisitUser(const MemberExpr *Member, bool OnConstObject) { + if (Member->isBoundMemberFunction(Ctxt)) { + if (!OnConstObject || Member->getFoundDecl().getAccess() != AS_public) { + // Non-public non-static member functions might not preserve the + // logical costness. E.g. in + // class C { + // int &data() const; + // public: + // int &get() { return data(); } + // }; + // get() uses a private const method, but must not be made const + // itself. + return false; // Stop traversal. + } + // Using a public non-static const member function. + return true; + } + + const auto *Parent = getParent(Member); + + if (const auto *Cast = dyn_cast_or_null(Parent)) { + // A read access to a member is safe when the member either + // 1) has builtin type (a 'const int' cannot be modified), + // 2) or it's a public member (the pointee of a public 'int * const' can + // can be modified by any user of the class). + if (Member->getFoundDecl().getAccess() != AS_public && + !Cast->getType()->isBuiltinType()) + return false; + + if (Cast->getCastKind() == CK_LValueToRValue) + return true; + + if (Cast->getCastKind() == CK_NoOp && Cast->getType().isConstQualified()) + return true; + } + + if (const auto *M = dyn_cast_or_null(Parent)) + return VisitUser(M, /*OnConstObject=*/false); + + return false; // Stop traversal. + } + + bool VisitCXXThisExpr(const CXXThisExpr *E) { + Usage = Const; + + const auto *Parent = getParent(E); + + // Look through deref of this. + if (const auto *UnOp = dyn_cast_or_null(Parent)) { + if (UnOp->getOpcode() == UO_Deref) { + Parent = getParent(UnOp); + } + } + + // It's okay to + // return (const S*)this; + // use((const S*)this); + // ((const S*)this)->f() + // when 'f' is a public member function. + if (const auto *Cast = dyn_cast_or_null(Parent)) { + if (VisitUser(Cast)) + return true; + + // And it's also okay to + // (const T)(S->t) + // (LValueToRValue)(S->t) + // when 't' is either of builtin type or a public member. + } else if (const auto *Member = dyn_cast_or_null(Parent)) { + if (VisitUser(Member, /*OnConstObject=*/false)) + return true; + } + + // Unknown user of this. + Usage = NonConst; + return false; // Stop traversal. + } +}; + +AST_MATCHER(CXXMethodDecl, usesThisAsConst) { + FindUsageOfThis UsageOfThis(Finder->getASTContext()); + + // TraverseStmt does not modify its argument. + UsageOfThis.TraverseStmt(const_cast(Node.getBody())); + + return UsageOfThis.Usage == Const; +} + +void MakeMemberFunctionConstCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + Finder->addMatcher( + cxxMethodDecl( + isDefinition(), isUserProvided(), + unless(anyOf( + isExpansionInSystemHeader(), isVirtual(), isConst(), isStatic(), + hasTrivialBody(), cxxConstructorDecl(), cxxDestructorDecl(), + isTemplate(), isDependentContext(), + ofClass(anyOf( + isLambda(), + hasAnyDependentBases()) // Method might become virtual + // depending on template base class. + ), + isInsideMacroDefinition(), + hasCanonicalDecl(isInsideMacroDefinition()))), + usesThisAsConst()) + .bind("x"), + this); +} + +static SourceLocation getConstInsertionPoint(const CXXMethodDecl *M) { + TypeSourceInfo *TSI = M->getTypeSourceInfo(); + if (!TSI) + return {}; + + FunctionTypeLoc FTL = + TSI->getTypeLoc().IgnoreParens().getAs(); + if (!FTL) + return {}; + + return FTL.getRParenLoc().getLocWithOffset(1); +} + +void MakeMemberFunctionConstCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Definition = Result.Nodes.getNodeAs("x"); + + auto Declaration = Definition->getCanonicalDecl(); + + 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 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 @@ -23,6 +23,7 @@ #include "InconsistentDeclarationParameterNameCheck.h" #include "IsolateDeclarationCheck.h" #include "MagicNumbersCheck.h" +#include "MakeMemberFunctionConstCheck.h" #include "MisleadingIndentationCheck.h" #include "MisplacedArrayIndexCheck.h" #include "NamedParameterCheck.h" @@ -79,6 +80,8 @@ "readability-isolate-declaration"); CheckFactories.registerCheck( "readability-magic-numbers"); + CheckFactories.registerCheck( + "readability-make-member-function-const"); CheckFactories.registerCheck( "readability-misleading-indentation"); 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 @@ -147,6 +147,12 @@ Finds classes, structs, and unions that contain redundant member access specifiers. +- New :doc:`readability-make-member-function-const + ` check. + + Finds non-static member functions that can be made ``const`` + because the functions don't use ``this`` in a non-const way. + 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 @@ -361,6 +361,7 @@ readability-inconsistent-declaration-parameter-name readability-isolate-declaration readability-magic-numbers + readability-make-member-function-const readability-misleading-indentation readability-misplaced-array-index readability-named-parameter diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst @@ -0,0 +1,67 @@ +.. title:: clang-tidy - readability-make-member-function-const + +readability-make-member-function-const +====================================== + +Finds non-static member functions that can be made ``const`` +because the functions don't use ``this`` in a non-const way. + +This check tries to annotate methods according to +`logical constness `_ +(not physical constness). +Therefore, it will suggest to add a ``const`` qualifier to a non-const +method only if this method does something that is already possible though the +public interface on a ``const`` pointer to the object: + +* reading a public member variable +* calling a public const-qualified member function +* returning const-qualified ``this`` +* passing const-qualified ``this`` as a parameter. + +This check will also suggest to add a ``const`` qualifier to a non-const +method if this method uses private data and functions in a limited number of +ways where logical constness and physical constness coincide: + +* reading a member variable of builtin type + +Specifically, this check will not suggest to add a ``const`` to a non-const +method if the method reads a private member variable of pointer type because +that allows to modify the pointee which might not preserve logical constness. +For the same reason, it does not allow to call private member functions +or member functions on private member variables. + +In addition, this check ignores functions that + +* are declared ``virtual`` +* contain a ``const_cast`` +* are templated or part of a class template +* have an empty body +* do not (implicitly) use ``this`` at all + (see `readability-convert-member-functions-to-static `_). + +The following real-world examples will be preserved by the check: + +.. code-block:: c++ + + class E1 { + Pimpl &getPimpl() const; + public: + int &get() { + // Calling a private member function disables this check. + return getPimpl()->i; + } + ... + }; + + class E2 { + public: + const int *get() const; + // const_cast disables this check. + S *get() { + return const_cast(const_cast(this)->get()); + } + ... + }; + +After applying modifications as suggested by the check, runnnig the check again +might find more opportunities to mark member functions ``const``. diff --git a/clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp b/clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp @@ -0,0 +1,332 @@ +// RUN: %check_clang_tidy %s readability-make-member-function-const %t + +struct Str { + void const_method() const; + void non_const_method(); +}; + +namespace Diagnose { +struct A; + +void free_const_use(const A *); +void free_const_use(const A &); + +struct A { + int M; + const int ConstM; + struct { + int M; + } Struct; + Str S; + Str &Sref; + + void already_const() const; + + int read_field() { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const + // CHECK-FIXES: {{^}} int read_field() const { + return M; + } + + int read_struct_field() { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const + // CHECK-FIXES: {{^}} int read_struct_field() const { + return Struct.M; + } + + 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 ConstM; + } + + void call_const_member() { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const + // CHECK-FIXES: {{^}} void call_const_member() const { + already_const(); + } + + void call_const_member_on_public_field() { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const + // CHECK-FIXES: {{^}} void call_const_member_on_public_field() const { + S.const_method(); + } + + void call_const_member_on_public_field_ref() { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const + // CHECK-FIXES: {{^}} void call_const_member_on_public_field_ref() const { + Sref.const_method(); + } + + const Str &return_public_field_ref() { + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const + // CHECK-FIXES: {{^}} const Str &return_public_field_ref() const { + return S; + } + + const A *return_this_const() { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const + // CHECK-FIXES: {{^}} const A *return_this_const() const { + return this; + } + + const A &return_this_const_ref() { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const + // CHECK-FIXES: {{^}} const A &return_this_const_ref() const { + return *this; + } + + void const_use() { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const + // CHECK-FIXES: {{^}} void const_use() const { + free_const_use(this); + } + + void const_use_ref() { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const + // CHECK-FIXES: {{^}} void const_use_ref() const { + free_const_use(*this); + } + + auto trailingReturn() -> int { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const + // CHECK-FIXES: {{^}} auto trailingReturn() const -> int { + return M; + } + + int volatileFunction() volatile { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const + // CHECK-FIXES: {{^}} int volatileFunction() const volatile { + return M; + } + + int restrictFunction() __restrict { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const + // CHECK-FIXES: {{^}} int restrictFunction() const __restrict { + return M; + } + + int refFunction() & { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const + // CHECK-FIXES: {{^}} int refFunction() const & { + return M; + } + + void out_of_line_call_const(); + // CHECK-FIXES: {{^}} void out_of_line_call_const() const; +}; + +void A::out_of_line_call_const() { + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: method 'out_of_line_call_const' can be made const + // CHECK-FIXES: {{^}}void A::out_of_line_call_const() const { + already_const(); +} +} // namespace Diagnose + +namespace Keep { +struct Keep; +void free_non_const_use(Keep *); +void free_non_const_use(Keep &); + +struct Keep { +private: + void private_const_method() const; + Str PrivateS; + Str *Sptr; + Str &Sref; + +public: + int M; + Str S; + + void keepTrivial() {} + + // See readability-convert-member-functions-to-static instead. + void keepStatic() { int I = 0; } + + const int *keepConstCast() const; + int *keepConstCast() { // Needs to stay non-const. + return const_cast(static_cast(this)->keepConstCast()); + } + + void non_const_use() { free_non_const_use(this); } + void non_const_use_ref() { free_non_const_use(*this); } + + Keep *return_this() { + return this; + } + + Keep &return_this_ref() { + return *this; + } + + void escape_this() { + Keep *Escaped = this; + } + + void call_private_const_method() { + private_const_method(); + } + + int keepConst() const { return M; } + + virtual int keepVirtual() { return M; } + + void writeField() { + M = 1; + } + + void callNonConstMember() { writeField(); } + + void call_non_const_member_on_field() { S.non_const_method(); } + + void call_const_member_on_private_field() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + PrivateS.const_method(); + } + + const Str &return_private_field_ref() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + return PrivateS; + } + + 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; + } + + const Str *return_const_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 escaped_private_field() { + const auto &Escaped = Sref; + } + + Str &return_field_ref() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + return Sref; + } + + const Str &return_field_const_ref() { + // Technically, this method could be const-qualified, + // but it might not be logically const. + return Sref; + } +}; + +struct KeepVirtualDerived : public Keep { + int keepVirtual() { return M; } +}; + +void KeepLambdas() { + auto F = +[]() { return 0; }; + auto F2 = []() { return 0; }; +} + +template +struct KeepWithDependentBase : public Base { + int M; + // We cannot make this method const because it might need to override + // a function from Base. + int const_f() { return M; } +}; + +template +struct KeepClassTemplate { + int M; + // We cannot make this method const because a specialization + // might use *this differently. + int const_f() { return M; } +}; + +struct KeepMemberFunctionTemplate { + int M; + // We cannot make this method const because a specialization + // might use *this differently. + template + int const_f() { return M; } +}; + +void instantiate() { + struct S {}; + KeepWithDependentBase I1; + I1.const_f(); + + KeepClassTemplate I2; + I2.const_f(); + + KeepMemberFunctionTemplate I3; + I3.const_f(); +} + +struct NoFixitInMacro { + int M; + +#define FUN const_use_macro() + int FUN { + return M; + } + +#define T(FunctionName, Keyword) \ + int FunctionName() Keyword { return M; } +#define EMPTY + T(A, EMPTY) + T(B, const) + +#define T2(FunctionName) \ + int FunctionName() { return M; } + T2(A2) +}; + +// Real-world code, see clang::ObjCInterfaceDecl. +class DataPattern { + int &data() const; + +public: + const int &get() const { + return const_cast(this)->get(); + } + + // This member function must stay non-const, even though + // it only calls other private const member functions. + int &get() { + return data(); + } + + void set() { + data() = 42; + } +}; + +struct MemberFunctionPointer { + void call_non_const(void (MemberFunctionPointer::*FP)()) { + (this->*FP)(); + } + + void call_const(void (MemberFunctionPointer::*FP)() const) { + (this->*FP)(); + } +}; + +} // namespace Keep