[PATCH] D20053: [clang-tidy] Add misc-unnecessary-mutable check.

[Etienne Bergeron via cfe-commits]

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<FieldUseVisitor> {
+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<MemberExpr>(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<Expr>(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<Expr>(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