[PATCH] D25316: [clang-tidy] Enhance modernize-use-auto to casts

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:346
 
-    const auto *NewExpr = cast<CXXNewExpr>(V->getInit()->IgnoreParenImpCasts());
-    // Ensure that every VarDecl has a CXXNewExpr initializer.
-    if (!NewExpr)
+    const auto *Expr = cast<ExprType>(V->getInit()->IgnoreParenImpCasts());
+    // Ensure that every VarDecl has an ExprType initializer.
----------------
I think you should use `dyn_cast` here because `ExprType` may vary from call to call and produce an invalid cast otherwise.


================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:404
                  Result.Nodes.getNodeAs<DeclStmt>(DeclWithNewId)) {
-    replaceNew(Decl, Result.Context);
+    auto GetType = [](const CXXNewExpr *Expr) {
+      return Expr->getType();
----------------
Just inline the lambda expression (below as well).


================
Comment at: clang-tidy/modernize/UseAutoCheck.h:28
   void replaceIterators(const DeclStmt *D, ASTContext *Context);
-  void replaceNew(const DeclStmt *D, ASTContext *Context);
+  template <typename ExprType, typename TypeFn>
+  void replaceExpr(const DeclStmt *D, ASTContext *Context, TypeFn GetType,
----------------
Having the template declaration in the header, but the definition in the source file, is living a bit too close to the edge for my tastes. Since there are no function definitions in the header file, I suppose this may be okay, however.


================
Comment at: clang-tidy/modernize/UseAutoCheck.h:30
+  void replaceExpr(const DeclStmt *D, ASTContext *Context, TypeFn GetType,
+                   StringRef message);
 
----------------
s/message/Message


================
Comment at: docs/clang-tidy/checks/modernize-use-auto.rst:150
+Frequently, when a variable is declared and initialized with a cast, the
+variable type has to be written twice: in the declaration type and in the
+cast expression. In this cases, the declaration type can be replaced with
----------------
s/has to be/is


================
Comment at: docs/clang-tidy/checks/modernize-use-auto.rst:164-165
+``reinterpret_cast``, functional casts and c style casts.
+It does not handle function templates that behave as casts, such as
+``llvm::dyn_cast``, ``boost::lexical_cast`` or ``gsl::narrow_cast``.
+
----------------
Should this be moved into Known Limitations?


https://reviews.llvm.org/D25316