alexfh added inline comments. ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:45 @@ +44,3 @@ + + // The following test is based on DeclPrinter::VisitVarDecl() o find if an + // initializer is implicit o not. ---------------- s/o find/to find/?
s/o not/or not/? ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:51 @@ +50,3 @@ + return false; + ImplicitInit = Construct->getNumArgs() == 0 || + Construct->getArg(0)->isDefaultArgument(); ---------------- 1. The logic is somewhat convoluted here. Instead of using a boolean flag that's assigned only here, I'd directly return the value. 2. Don't use else after return. http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return if (const auto *Construct = dyn_cast<CXXConstructExpr>(Init)) { return Construct->isListInitialization() && Construct->getNumArgs() > 0 && !Construct->getArg(0)->isDefaultArgument(); } return Node.getInitStyle() != VarDecl::ListInit; ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:81 @@ +80,3 @@ + if (NewQT == QT) + break; + QT = NewQT; ---------------- Replace `break;` with `return false;` to make the control flow easier to understand. ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:180 @@ +179,3 @@ + return typedefType( + hasDeclaration(allOf(namedDecl(hasStdIteratorName()), + hasDeclContext(recordDecl(hasStdContainerName(), ---------------- The `hasDeclaration(...)` part is also used in the next function. Consider pulling this to a separate function. ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:206 @@ +205,3 @@ + namesType( + anyOf(typedefType(hasDeclaration(namedDecl(hasStdIteratorName()))), + recordType(hasDeclaration(namedDecl(hasStdIteratorName()))))))); ---------------- `hasDeclaration(namedDecl(hasStdIteratorName()))` is used at least twice. Please consider pulling this to a variable. ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:348 @@ +347,3 @@ + FirstDecl->getTypeSourceInfo()->getTypeLoc().getSourceRange()); + auto Diag = diag(Range.getBegin(), "use auto"); + ---------------- I'd make the message more user-friendly by specifying instead of what exactly `auto` should be used. This could be unclear from the context what exactly the check means. ================ Comment at: clang-tidy/modernize/UseAutoCheck.cpp:361 @@ +360,3 @@ +void UseAutoCheck::check(const MatchFinder::MatchResult &Result) { + const DeclStmt *Dec; + if ((Dec = Result.Nodes.getNodeAs<DeclStmt>(IteratorDeclStmtId))) { ---------------- I'd rewrite this to use the `if (T *x = ...)` style: if (const auto *Decl = Result.Nodes.getNodeAs<DeclStmt>(IteratorDeclStmtId)) { ReplaceIterators(Decl, Result.Context); } else if (const auto *Decl = Result.Nodes.getNodeAs<DeclStmt>(DeclWithNewId)) { ReplaceNew(Dec, Result.Context); } else { llvm_unreachable("Bad Callback. No node provided."); } ================ Comment at: clang-tidy/modernize/UseAutoCheck.h:27 @@ +26,3 @@ + + void ReplaceIterators(const DeclStmt *D, ASTContext *Context); + void ReplaceNew(const DeclStmt *D, ASTContext *Context); ---------------- Why are these public? ================ Comment at: clang-tidy/modernize/UseAutoCheck.h:27 @@ +26,3 @@ + + void ReplaceIterators(const DeclStmt *D, ASTContext *Context); + void ReplaceNew(const DeclStmt *D, ASTContext *Context); ---------------- alexfh wrote: > Why are these public? Method names should start with a lower-case character. http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly ================ Comment at: test/clang-tidy/modernize-use-auto-iterator.cpp:30 @@ +29,3 @@ + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto [modernize-use-auto] + // CHECK-FIXES: auto I1 = C.begin(); + ---------------- Make the variable names unique across all functions to avoid incorrect matches. http://reviews.llvm.org/D12231 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
