Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
etienneb added inline comments. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:23 @@ +22,3 @@ + +// Matcher checking if the declaration is non-macro existent mutable which is +// not dependent on context. add an anonymous namespace around this declaration. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:26 @@ +25,3 @@ +AST_MATCHER(FieldDecl, isSubstantialMutable) { + return Node.getDeclName() && Node.isMutable() && + !Node.getLocation().isMacroID() && why getDeclName ? Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:42 @@ +41,3 @@ + +class FieldUseVisitor : public RecursiveASTVisitor { +public: Please add a comment to describe the purpose of this class. The code is the doc, and it will help readers. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:56 @@ +55,3 @@ + bool VisitExpr(Expr *GenericExpr) { +MemberExpr *Expression = dyn_cast(GenericExpr); +if (!Expression || Expression->getMemberDecl() != SoughtField) MemberExpr *Expression -> const auto* Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:60 @@ +59,3 @@ + +// Check if expr is a member of const thing. +bool IsConstObj = false; thing -> object Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:62 @@ +61,3 @@ +bool IsConstObj = false; +for (const auto *ChildStmt : Expression->children()) { + const Expr *ChildExpr = dyn_cast(ChildStmt); I think the child-expressions of MemberExpr can only be "member->getBase()" ? In this case, no loop is needed. ``` child_range children() { return child_range(&Base, &Base+1); } ``` ``` Expr *getBase() const { return cast(Base); } ``` Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:134 @@ +133,3 @@ +// All decls need their definitions in main file. +if (!Declaration->hasBody() || +!SM.isInMainFile(Declaration->getBody()->getLocStart())) { Watch out, Declaration->hasBody() is tricky with code when compile on windows (clang-cl). hasBody() may return true, but the getBody() may be NULL. This is why these tests exist: ``` cppcoreguidelines-pro-type-member-init-delayed.cpp modernize-redundant-void-arg-delayed.cpp modernize-use-default-delayed.cpp performance-unnecessary-value-param-delayed.cpp ``` So this code may crash with delayed-template-parsing. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:213 @@ +212,3 @@ + diag(FD->getLocation(), "'mutable' modifier is unnecessary for field %0") + << FD->getDeclName(); + no need for ->getDeclName() There is an overloaded operator for namedDecl. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:217 @@ +216,3 @@ + + if (CheckRemoval(SM, FD->getLocStart(), FD->getLocEnd(), Context, + RemovalRange)) { You can change CheckRemoval to return the SourceRange. If it's failing, you can call 'fixithint <<' and it won't be an issue. This way, you do not need to declare Diag, and you can directly add the sourceRange to the expression to line 213. ``` diag(FD->getLocation(), "'mutable' modifier is unnecessary for field %0") << FD << getSourceRangeOfMutableToken(FD); ``` WDYT? http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
mnbvmar marked 14 inline comments as done. mnbvmar added a comment. http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
mnbvmar added inline comments. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:47 @@ +46,3 @@ + + void RunSearch(const Decl *Declaration) { +auto *Body = Declaration->getBody(); Unless I miss something, the moment we set FoundNonConstUse to true, we stop recurring both in FieldUseVisitor and ClassMethodVisitor. http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
mnbvmar updated this revision to Diff 59445. mnbvmar added a comment. Fixes done. Added macro test. Docs should be working now. Updated docs. http://reviews.llvm.org/D20053 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/UnnecessaryMutableCheck.cpp clang-tidy/misc/UnnecessaryMutableCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-unnecessary-mutable.rst test/clang-tidy/misc-unnecessary-mutable.cpp 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
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
aaron.ballman added inline comments. Comment at: test/clang-tidy/misc-unnecessary-mutable.cpp:237 @@ +236,3 @@ + +// Fails for now. +/* It would be good to put further information in about why this fails. http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
etienneb added a subscriber: etienneb. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:38 @@ +37,3 @@ + + void RunSearch(Decl *Declaration) { +auto *Body = Declaration->getBody(); nit: Decl *Declaration -> const Decl *Declaration and other visitor functions. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:40 @@ +39,3 @@ +auto *Body = Declaration->getBody(); +ParMap = new ParentMap(Body); +TraverseStmt(Body); Why not using stack memory instead of allocating memory? ParentMap LocalMap; ParMap = &LocalMap; TraverseStmt(Body); ParMap = nullptr;/// <-- I also prefer seeing this variable being after recursion. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:46 @@ +45,3 @@ + bool VisitExpr(Expr *GenericExpr) { +MemberExpr *Expression = dyn_cast(GenericExpr); +if (Expression == nullptr) Could we shortcut the recursion if "FoundNonConstUse" is true. There is already a case found. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:50 @@ +49,3 @@ + +if (Expression->getMemberDecl() != SoughtField) + return true; this "if" and the previous one should be merged. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:66 @@ +65,3 @@ +// It's something weird. Just to be sure, assume we're const. +IsConstObj = true; + } else { As soon as something is "IsConstObj", they must be an early exit. No need to continue iterations. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:80 @@ +79,3 @@ +// whose parent is ImplicitCastExpr to rvalue or something constant. +bool HasRvalueCast = false; +bool HasConstCast = false; nit: HasRvalueCast -> HasRValueCast Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:85 @@ +84,3 @@ + dyn_cast(ParMap->getParent(Expression)); + if (Cast != nullptr) { +HasRvalueCast = Cast->getCastKind() == CastKind::CK_LValueToRValue; if (Cast != nullptr) { replace by if (const auto* Cast = dyn_cast(ParMap->getParent(Expression)) { and remove previous line. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:99 @@ +98,3 @@ + + bool IsNonConstUseFound() const { return FoundNonConstUse; } + nit: IsNonConstUseFound -> isNonConstUseFound coding style wants lower case as first letter. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:121 @@ +120,3 @@ +FunctionDecl *Declaration = dyn_cast(GenericDecl); + +if (Declaration == nullptr) nit: remove this empty line Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:167 @@ +166,3 @@ + +if (DeclToken.getKind() == tok::TokenKind::semi) { + break; nit: remove { } Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:171 @@ +170,3 @@ + +if (DeclToken.getKind() == tok::TokenKind::comma) { + return false; nit: remove { } http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
Prazek added a reviewer: Prazek. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:152 @@ +151,3 @@ +// it is the only declaration in a declaration chain. +static bool CheckRemoval(SourceManager &SM, const SourceLocation &LocStart, + const SourceLocation &LocEnd, ASTContext &Context, I guess you can use hasSingleDecl in matcher to solve this http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. One quick thought: we should probably have a test that includes a macro to make sure nothing explodes. e.g., #define FIELD(T, N) mutable T N class C { FIELD(int, RemoveMutable); FIELD(int, KeepMutable) public: void haha() const { KeepMutable = 12; } }; Not that I expect to run into this case often, but it would be good to ensure the check doesn't crash or attempt to modify the macro. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:194 @@ +193,3 @@ +void UnnecessaryMutableCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MD = Result.Nodes.getNodeAs("field"); + const auto *ClassMatch = dyn_cast(MD->getParent()); Why MD; that's usually a method decl. Comment at: clang-tidy/misc/UnnecessaryMutableCheck.cpp:200 @@ +199,3 @@ + if (!MD->getDeclName() || ClassMatch->isDependentContext() || + !MD->isMutable()) +return; I think this would be more useful to hoist into the matcher itself (which may mean adding a new AST matcher first, if there isn't already one). http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
alexfh added a subscriber: alexfh. Comment at: docs/clang-tidy/checks/misc-unnecessary-mutable.rst:9 @@ +8,3 @@ + +.. code-block:: c++ + class SomeClass { Please verify the documentation can be built without errors. On Ubuntu this boils down to: Install sphinx: 1. `$ sudo apt-get install sphinx-common` 2. Enable `LLVM_BUILD_DOCS` and maybe some other options in cmake. 2. `$ ninja docs-clang-tools-html` (or something similar, if you use make). http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
mnbvmar created this revision. mnbvmar added a reviewer: alexfh. mnbvmar added subscribers: krystyna, sbarzowski, Prazek, staronj, cfe-commits. This implements unnecessary-mutable check. It's still bug-prone and might produce false positives, so all suggestions are welcome. http://reviews.llvm.org/D20053 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/UnnecessaryMutableCheck.cpp clang-tidy/misc/UnnecessaryMutableCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-unnecessary-mutable.rst test/clang-tidy/misc-unnecessary-mutable.cpp Index: test/clang-tidy/misc-unnecessary-mutable.cpp === --- /dev/null +++ test/clang-tidy/misc-unnecessary-mutable.cpp @@ -0,0 +1,354 @@ +// 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 doSomethingC
Re: [PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.
Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). http://reviews.llvm.org/D20053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits