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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits