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

[Etienne Bergeron via cfe-commits]

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<MemberExpr>(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<ImplicitCastExpr>(ParMap->getParent(Expression));
+      if (Cast != nullptr) {
+        HasRvalueCast = Cast->getCastKind() == CastKind::CK_LValueToRValue;
----------------
if (Cast != nullptr) {

replace by

if (const auto* Cast =  dyn_cast<ImplicitCastExpr>(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<FunctionDecl>(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