[PATCH] D12231: Add use-auto check to modernize module.

[Alexander Kornienko via cfe-commits]

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