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(, +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 ) 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 =
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 , const SourceLocation , + const SourceLocation , ASTContext , 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 ) {
+ 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
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
