Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -37,6 +37,7 @@ ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp UniqueptrResetReleaseCheck.cpp + UnnecessaryMutableCheck.cpp UnusedAliasDeclsCheck.cpp UnusedParametersCheck.cpp UnusedRAIICheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -45,6 +45,7 @@ #include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" #include "UniqueptrResetReleaseCheck.h" +#include "UnnecessaryMutableCheck.h" #include "UnusedAliasDeclsCheck.h" #include "UnusedParametersCheck.h" #include "UnusedRAIICheck.h" @@ -126,6 +127,8 @@ "misc-undelegated-constructor"); CheckFactories.registerCheck( "misc-uniqueptr-reset-release"); + CheckFactories.registerCheck( + "misc-unnecessary-mutable"); CheckFactories.registerCheck( "misc-unused-alias-decls"); CheckFactories.registerCheck( Index: clang-tidy/misc/UnnecessaryMutableCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/UnnecessaryMutableCheck.h @@ -0,0 +1,43 @@ +//===--- UnnecessaryMutableCheck.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_MISC_UNNECESSARY_MUTABLE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNNECESSARY_MUTABLE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Searches for and removes unnecessary 'mutable' keywords (that is, for +/// fields that cannot be possibly changed in const methods). +/// +/// Example: +/// +/// class MyClass { +/// mutable int field; // will change into "int field;" +/// }; +/// +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-unnecessary-mutable.html +class UnnecessaryMutableCheck : public ClangTidyCheck { +public: + UnnecessaryMutableCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNNECESSARY_MUTABLE_H Index: clang-tidy/misc/UnnecessaryMutableCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/UnnecessaryMutableCheck.cpp @@ -0,0 +1,225 @@ +//===--- UnnecessaryMutableCheck.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 "UnnecessaryMutableCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/ParentMap.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +// Matcher checking if the declaration is non-macro existent mutable which is +// not dependent on context. +AST_MATCHER(FieldDecl, isSubstantialMutable) { + return Node.getDeclName() && Node.isMutable() && + !Node.getLocation().isMacroID() && + !Node.getParent()->isDependentContext(); +} + +void UnnecessaryMutableCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + fieldDecl(anyOf(isPrivate(), allOf(isProtected(), + hasParent(cxxRecordDecl(isFinal())))), + unless(anyOf(isImplicit(), isInstantiated(), + hasParent(cxxRecordDecl(isUnion())))), + isSubstantialMutable()) + .bind("field"), + this); +} + +class FieldUseVisitor : public RecursiveASTVisitor { +public: + explicit FieldUseVisitor(const FieldDecl *SoughtField) + : SoughtField(SoughtField), FoundNonConstUse(false) {} + + void RunSearch(const Decl *Declaration) { + auto *Body = Declaration->getBody(); + ParentMap LocalMap(Body); + ParMap = &LocalMap; + TraverseStmt(Body); + ParMap = nullptr; + } + + bool VisitExpr(Expr *GenericExpr) { + MemberExpr *Expression = dyn_cast(GenericExpr); + if (!Expression || Expression->getMemberDecl() != SoughtField) + return true; + + // Check if expr is a member of const thing. + bool IsConstObj = false; + for (const auto *ChildStmt : Expression->children()) { + const Expr *ChildExpr = dyn_cast(ChildStmt); + if (ChildExpr) { + IsConstObj |= ChildExpr->getType().isConstQualified(); + + // If member expression dereferences, we need to check + // whether pointer type is const. + if (Expression->isArrow()) { + const auto &WrappedType = *ChildExpr->getType(); + if (!WrappedType.isPointerType()) { + // It's something weird. Just to be sure, assume we're const. + IsConstObj = true; + } else { + IsConstObj |= WrappedType.getPointeeType().isConstQualified(); + } + } + } + + if (IsConstObj) + break; + } + + // If it's not, mutableness changes nothing. + if (!IsConstObj) + return true; + + // By a const operation on a member expression we mean a MemberExpr + // whose parent is ImplicitCastExpr to rvalue or something constant. + bool HasRValueCast = false; + bool HasConstCast = false; + if (ParMap->hasParent(Expression)) { + if (const auto *Cast = + dyn_cast(ParMap->getParent(Expression))) { + HasRValueCast = Cast->getCastKind() == CastKind::CK_LValueToRValue; + HasConstCast = Cast->getType().isConstQualified(); + } + } + + if (!HasRValueCast && !HasConstCast) { + FoundNonConstUse = true; + return false; + } + + return true; + } + + bool isNonConstUseFound() const { return FoundNonConstUse; } + +private: + const FieldDecl *SoughtField; + ParentMap *ParMap; + bool FoundNonConstUse; +}; + +class ClassMethodVisitor : public RecursiveASTVisitor { +public: + ClassMethodVisitor(ASTContext &Context, const FieldDecl *SoughtField) + : SM(Context.getSourceManager()), SoughtField(SoughtField), + NecessaryMutable(false) {} + + bool VisitDecl(const Decl *GenericDecl) { + // As for now, we can't track friends. + if (dyn_cast(GenericDecl)) { + NecessaryMutable = true; + return false; + } + + const FunctionDecl *Declaration = dyn_cast(GenericDecl); + if (Declaration == nullptr) + return true; + + // All decls need their definitions in main file. + if (!Declaration->hasBody() || + !SM.isInMainFile(Declaration->getBody()->getLocStart())) { + NecessaryMutable = true; + return false; + } + + FieldUseVisitor FieldVisitor(SoughtField); + FieldVisitor.RunSearch(Declaration); + if (FieldVisitor.isNonConstUseFound()) { + NecessaryMutable = true; + return false; + } + + return true; + } + + bool IsMutableNecessary() const { return NecessaryMutable; } + +private: + SourceManager &SM; + const FieldDecl *SoughtField; + bool NecessaryMutable; +}; + +// Checks if 'mutable' keyword can be removed; for now; we do it only if +// it is the only declaration in a declaration chain. +static bool CheckRemoval(SourceManager &SM, const SourceLocation &LocStart, + const SourceLocation &LocEnd, ASTContext &Context, + SourceRange &ResultRange) { + + const FileID FID = SM.getFileID(LocEnd); + const llvm::MemoryBuffer *Buffer = SM.getBuffer(FID, LocEnd); + Lexer DeclLexer(SM.getLocForStartOfFile(FID), Context.getLangOpts(), + Buffer->getBufferStart(), SM.getCharacterData(LocStart), + Buffer->getBufferEnd()); + + Token DeclToken; + bool result = false; + + while (!DeclLexer.LexFromRawLexer(DeclToken)) { + + if (DeclToken.getKind() == tok::TokenKind::semi) + break; + + if (DeclToken.getKind() == tok::TokenKind::comma) + return false; + + if (DeclToken.isOneOf(tok::TokenKind::identifier, + tok::TokenKind::raw_identifier)) { + auto TokenStr = DeclToken.getRawIdentifier().str(); + + // "mutable" cannot be used in any other way than to mark mutableness + if (TokenStr == "mutable") { + ResultRange = + SourceRange(DeclToken.getLocation(), DeclToken.getEndLoc()); + result = true; + } + } + } + + assert(result && "No mutable found, weird"); + + return result; +} + +void UnnecessaryMutableCheck::check(const MatchFinder::MatchResult &Result) { + const auto *FD = Result.Nodes.getNodeAs("field"); + const auto *ClassMatch = dyn_cast(FD->getParent()); + auto &Context = *Result.Context; + auto &SM = *Result.SourceManager; + + ClassMethodVisitor Visitor(Context, FD); + Visitor.TraverseDecl(const_cast(ClassMatch)); + + if (Visitor.IsMutableNecessary()) + return; + + auto Diag = + diag(FD->getLocation(), "'mutable' modifier is unnecessary for field %0") + << FD->getDeclName(); + + SourceRange RemovalRange; + + if (CheckRemoval(SM, FD->getLocStart(), FD->getLocEnd(), Context, + RemovalRange)) { + Diag << FixItHint::CreateRemoval(RemovalRange); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -175,6 +175,11 @@ Find suspicious usage of runtime string comparison functions. +- New `misc-unnecessary-mutable + `_ check + + Finds places where ``mutable`` keyword is potentially unneeded. + - New `misc-unused-using-decls `_ check Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -86,6 +86,7 @@ misc-unconventional-assign-operator misc-undelegated-constructor misc-uniqueptr-reset-release + misc-unnecessary-mutable misc-unused-alias-decls misc-unused-parameters misc-unused-raii Index: docs/clang-tidy/checks/misc-unnecessary-mutable.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-unnecessary-mutable.rst @@ -0,0 +1,51 @@ +.. title:: clang-tidy - misc-unnecessary-mutable + +misc-unnecessary-mutable +======================== + +The tool tries to find situations where ``mutable`` might be unnecessary +(i.e. each use of the mutable field on the const object is constant). + +.. code-block:: c++ + + class SomeClass { + public: + void foo() { + field2 = 42; + } + + void bar() const { + field3 = 99; + } + + // Should stay mutable. Everyone can change it. + mutable int field1; + + private: + // Might lose 'mutable'. It's never modified in const object. + mutable int field2; + + // Should stay mutable; bar() changes it even though 'this' is const. + mutable int field3; + }; + +It also can automatically remove ``mutable`` keyword. For now, it is done +only if non-necessarily mutable field is the only declaration in the statement. + +Please note that there is a possibility that false-positives occur. For example, +one can use ``const_cast``: + +.. code-block:: c++ + + class AnotherClass { + public: + void foo() const { + const int& y = x; + const_cast(y) = 42; + } + + private: + mutable int x; // SHOULD stay mutable. For now, it does not. + }; + +The tool should be thus used with care. Index: test/clang-tidy/misc-unnecessary-mutable.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-unnecessary-mutable.cpp @@ -0,0 +1,377 @@ +// RUN: %check_clang_tidy %s misc-unnecessary-mutable %t + +struct NothingMutable { + int field1; + unsigned field2; + const int field3; + volatile float field4; + + NothingMutable(int a1, unsigned a2, int a3, float a4) : field1(a1), field2(a2), field3(a3), field4(a4) {} + + void doSomething() { + field1 = 1; + field2 = 2; + field4 = 4; + } +}; + +struct NoMethods { + int field1; + mutable unsigned field2; // These cannot be fixed; they're public + const int field3; + mutable volatile NothingMutable field4; +}; + +class NoMethodsClass { +public: + int field1; + mutable unsigned field2; + +private: + const int field3; + mutable volatile NothingMutable field4; + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'mutable' modifier is unnecessary for field 'field4' [misc-unnecessary-mutable] + // CHECK-FIXES: {{^ }}volatile NothingMutable field4; +}; + +struct PrivateInStruct { +private: + mutable volatile unsigned long long blah; + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: 'mutable' modifier is unnecessary for field 'blah' {{..}} + // CHECK-FIXES: {{^ }}volatile unsigned long long blah; +}; + +union PrivateInUnion { +public: + int someField; + +private: + mutable char otherField; +}; + +class UnusedVar { +private: + mutable int x __attribute__((unused)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mutable' modifier is unnecessary for field 'x' {{..}} + // CHECK-FIXES: {{^ }}int x __attribute__((unused)); +}; + +class NoConstMethodsClass { +public: + int field1; + mutable unsigned field2; + + NoConstMethodsClass() : field2(42), field3(9), field4(NothingMutable(1, 2, 3, 4)) {} + + void doSomething() { + field2 = 8; + field1 = 99; + field4.doSomething(); + } + +private: + const int field3; + mutable NothingMutable field4; + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'mutable' modifier is unnecessary for field 'field4' {{..}} + // CHECK-FIXES: {{^ }}NothingMutable field4; +}; + +class ConstMethods { +private: + mutable int field1, field2; + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'mutable' modifier is unnecessary for field 'field2' {{..}} + mutable int incr, decr, set, mul, constArg1, constArg2, constRef, ref1, ref2; + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'mutable' modifier is unnecessary for field 'constArg1' {{..}} + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: 'mutable' modifier is unnecessary for field 'constArg2' {{..}} + // CHECK-MESSAGES: :[[@LINE-3]]:59: warning: 'mutable' modifier is unnecessary for field 'constRef' {{..}} + + void takeArg(int x) const { x *= 4; } + int takeConstRef(const int &x) const { return x + 99; } + void takeRef(int &) const {} + + template + void takeArgs(Args... args) const {} + template + void takeArgRefs(Args &... args) const {} + +public: + void doSomething() const { + field1 = field2; + } + + void doOtherThing() const { + incr++; + decr--; + set = 42; + mul *= 3; + takeArg(constArg1); + takeConstRef(constRef); + takeRef(ref1); + takeArgs(constArg1, constArg2); + takeArgRefs(ref1, ref2); + } +}; + +class NonFinalClass { +public: + mutable int fPublic; + +protected: + mutable int fProtected; + +private: + mutable int fPrivate; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mutable' modifier is unnecessary for field 'fPrivate' {{..}} + // CHECK-FIXES: {{^ }}int fPrivate; +}; + +class FinalClass final { +public: + mutable int fPublic; + +protected: + mutable int fProtected; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mutable' modifier is unnecessary for field 'fProtected' {{..}} + // CHECK-FIXES: {{^ }}int fProtected; + +private: + mutable int fPrivate; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mutable' modifier is unnecessary for field 'fPrivate' {{..}} + // CHECK-FIXES: {{^ }}int fPrivate; +}; + +class NotAllFuncsKnown { + void doSomething(); + void doSomethingConst() const {} + +private: + mutable int field; + // Can't be fixed. We don't know if doSomething() doesn't declare a *const* NotAllFuncsKnown instance + // and then modify 'field' field. +}; + +class NotAllConstFuncsKnown { + void doSomething() {} + void doSomethingConst() const; + void doOtherConst() const {} + +private: + mutable int field; +}; + +class ConstFuncOutside { + void doSomething(); + void doSomethingConst() const; + void doOtherConst() const {} + +private: + mutable int field; +}; + +void ConstFuncOutside::doSomethingConst() const {} + +// We can't really see if mutable is necessary or not. +template +class TemplatedClass { +public: + void doSomething() { + a = b = c; + } + + void doSomethingConst() const { + a = b = c; + } + +private: + mutable int a, b, c; +}; + +TemplatedClass TemplatedClassInt; + +class ClassWithTemplates { +public: + void doSomethingConst() const { + a = b; + } + + // Here, we can see that. + template + void doOtherConst() const { + b = c; + } + +private: + mutable int a, b, c, d; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'mutable' modifier is unnecessary for field 'c' {{..}} + // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 'mutable' modifier is unnecessary for field 'd' {{..}} +}; + +class ImplicitCast { +public: + void doSomethingConst() const { + a = b; + } + +private: + mutable int a; + mutable double b; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'mutable' modifier is unnecessary for field 'b' {{..}} + // CHECK_FIXES: {{^ }}double b; +}; + +struct MutableNotFirst { +private: + long mutable long abc = 42; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'mutable' modifier is unnecessary for field 'abc' {{..}} + // CHECK_FIXES: {{^ }}long long abc; + long long mutable bca; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'mutable' modifier is unnecessary for field 'bca' {{..}} + // CHECK_FIXES: {{^ }}long long bca; + int mutable ca; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'mutable' modifier is unnecessary for field 'ca' {{..}} + // CHECK_FIXES: {{^ }}int ca; +}; + +// Fails for now. (See: documentation.) +/* +void change(const int &a) { + const_cast(a) = 42; +} + +struct Change { + void changeMember() const { + change(m); + const int& x = otherM; + const_cast(x) = 33; + + const_cast(anotherM) = -4; + } +private: + mutable int m; + mutable int otherM; + mutable int anotherM; +}; +*/ + +class StrangeClass { +public: + void foo() {} +}; + +void EvilFunction(int &a) {} +void EvilFunction(const int &a) {} + +class JustClass { +public: + JustClass() {} + + void foo() { + MutableClass.foo(); + } + + void foo() const; + void evilCaller() const; + +private: + mutable StrangeClass MutableClass; // Must stay mutable (because of foo func below) + mutable int MutableInt; // Must stay mutable +}; + +void JustClass::foo() const { + MutableClass.foo(); +} + +void JustClass::evilCaller() const { + EvilFunction(MutableInt); +} + +class AnotherStrangeClass { +public: + // Example of non-const method which requires that MutableInt should stay mutable. + void strangeFoo() { + const AnotherStrangeClass *ConstThis = this; + EvilFunction(ConstThis->MutableInt); + } + +private: + mutable int MutableInt; // must stay mutable +}; + +class ClassWithStrangeConstructor { +public: + ClassWithStrangeConstructor() { + const ClassWithStrangeConstructor *ConstThis = this; + EvilFunction(ConstThis->MutableInt); + } + +private: + mutable int MutableInt; // must stay mutable +}; + +class ClassWithStrangeDestructor { +public: + ~ClassWithStrangeDestructor() { + const ClassWithStrangeDestructor *ConstThis = this; + EvilFunction(ConstThis->MutableInt); + } + +private: + mutable int MutableInt; // must stay mutable +}; + +// Don't touch friends or they'll hurt you. +class ClassWithFriends { +public: + friend void someFriend(ClassWithFriends *); + +private: + mutable int MutableInt; // must stay mutable +}; + +void someFriend(ClassWithFriends *Class) { + const ClassWithFriends *ConstClass = Class; + EvilFunction(ConstClass->MutableInt); +} + +class ClassWithClassFriends { +public: + friend class ClassFriend; + +private: + mutable int MutableInt; // must stay mutable +}; + +class ClassFriend { +public: + static void foo(const ClassWithClassFriends *Class) { + EvilFunction(Class->MutableInt); + } +}; + + +// Definitions containing macros. Nothing inside macro should +// be noticed nor modified. However, if 'mutable' keyword is outside +// of our macro, we leave the freedom to the check. It is not expected +// to notice and fix, but if it does (and it does once below), it is +// even better for us. +// This test was added mainly to figure out if nothing explodes inside the +// check. + +#define MUTABLE_FIELD(TYPE, NAME) mutable TYPE NAME +#define SOME_FIELD(TYPE, NAME) TYPE NAME +#define SOME_FIELD_SEMI(TYPE, NAME) TYPE NAME; +#define SOME_FIELD_TYPE(TYPE) TYPE +class MacroFields { + MUTABLE_FIELD(int, removedMutable); + MUTABLE_FIELD(int, keptMutable); + mutable SOME_FIELD(int, removedMutable2); + mutable SOME_FIELD_SEMI(int, removedMutable3) + mutable SOME_FIELD_TYPE(int) removedMutable4; + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'mutable' modifier is unnecessary for field 'removedMutable4' {{..}} + // CHECK-FIXES: {{^ }}SOME_FIELD_TYPE(int) removedMutable4; + +public: + void modify() const { + keptMutable = 44; + } +};