[clang-tidy] modernize-use-auto: don't remove stars by default

[alexfh]

Summary

By default, modernize-use-auto check will retain stars when replacing an explicit type with auto: MyType *t = new MyType; will be changed to auto *t = new MyType;, thus resulting in more consistency with the recommendations to use auto * for iterating over pointers in range-based for loops: http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

The new RemoveStars option allows to revert to the old behavior: with the new option turned on the check will change MyType *t = new MyType; to auto t = new MyType;.

[sbenza]

Is it a typo in the description when it says that when RemoveStar is on we will write 'auto*' instead if 'auto'?

[aaron.ballman]

The description threw me for a loop, but the behavior described in the documentation and tested by the tests matches what I would expect. LGTM!

[Eugene.Zelenko]

docs/clang-tidy/checks/modernize-use-auto.rst

155

Please highlight 0 with `. It's not language construct.

[alexfh]

• Updated formatting in the doc.

[alexfh]

In D20917#447046, @sbenza wrote:

Is it a typo in the description when it says that when RemoveStar is on we will write 'auto*' instead if 'auto'?

Yep, the whole patch description is a typo, fixed ;)

[Prazek]

besides it lgtm

clang-tidy/modernize/UseAutoCheck.cpp

338

Can you change this comment to make it more clear, or tell me what it do? I don't quite understand it right now

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

338

Will this be better?

// All subsequent variables in this declaration should have the same canonical type.
// For example, we don't want to use `auto` in `T *p = new T, **pp = new T*;`.

[Prazek]

clang-tidy/modernize/UseAutoCheck.cpp

338

Much better, thanks!

clang-tidy/modernize/UseAutoCheck.h

 #include "../ClangTidy.h"
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class UseAutoCheck : public ClangTidyCheck {
 public:
-  UseAutoCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
-
+  UseAutoCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void replaceIterators(const DeclStmt *D, ASTContext *Context);
   void replaceNew(const DeclStmt *D, ASTContext *Context);
+
+  const bool RemoveStars;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_AUTO_H

clang-tidy/modernize/UseAutoCheck.cpp

return declStmt(
                  // specifier source range which includes the identifier.
                  hasType(pointsTo(
                      pointsTo(parenType(innerType(functionType()))))))))))
       .bind(DeclWithNewId);
 }
 
 } // namespace
 
+UseAutoCheck::UseAutoCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RemoveStars(Options.get("RemoveStars", 0)) {}
+
+void UseAutoCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RemoveStars", RemoveStars ? 1 : 0);
+}
+
 void UseAutoCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (getLangOpts().CPlusPlus) {
     Finder->addMatcher(makeIteratorDeclMatcher(), this);
     Finder->addMatcher(makeDeclWithNewMatcher(), this);
   }
 }

 void UseAutoCheck::replaceNew(const DeclStmt *D, ASTContext *Context) {
   const auto *FirstDecl = dyn_cast<VarDecl>(*D->decl_begin());
   // Ensure that there is at least one VarDecl within the DeclStmt.
   if (!FirstDecl)
     return;
 
   const QualType FirstDeclType = FirstDecl->getType().getCanonicalType();
 
-  std::vector<SourceLocation> StarLocations;
+  std::vector<FixItHint> StarRemovals;
   for (const auto *Dec : D->decls()) {
     const auto *V = cast<VarDecl>(Dec);
     // Ensure that every DeclStmt child is a VarDecl.
     if (!V)
       return;
 
     const auto *NewExpr = cast<CXXNewExpr>(V->getInit()->IgnoreParenImpCasts());
     // Ensure that every VarDecl has a CXXNewExpr initializer.
     if (!NewExpr)
       return;
 
     // If VarDecl and Initializer have mismatching unqualified types.
     if (!Context->hasSameUnqualifiedType(V->getType(), NewExpr->getType()))
       return;
 
+    // All subsequent declarations should match the same non-decorated type.
+    if (FirstDeclType != V->getType().getCanonicalType())
+      return;
+
+    if (RemoveStars) {
       // Remove explicitly written '*' from declarations where there's more than
       // one declaration in the declaration list.
       if (Dec == *D->decl_begin())
         continue;
 
-    // All subsequent declarations should match the same non-decorated type.
-    if (FirstDeclType != V->getType().getCanonicalType())
-      return;
-
       auto Q = V->getTypeSourceInfo()->getTypeLoc().getAs<PointerTypeLoc>();
       while (!Q.isNull()) {
-      StarLocations.push_back(Q.getStarLoc());
+        StarRemovals.push_back(FixItHint::CreateRemoval(Q.getStarLoc()));
         Q = Q.getNextTypeLoc().getAs<PointerTypeLoc>();
       }
     }
+  }
 
   // FIXME: There is, however, one case we can address: when the VarDecl pointee
   // is the same as the initializer, just more CV-qualified. However, TypeLoc
   // information is not reliable where CV qualifiers are concerned so we can't
   // do anything about this case for now.
-  SourceRange Range(
-      FirstDecl->getTypeSourceInfo()->getTypeLoc().getSourceRange());
+  TypeLoc Loc = FirstDecl->getTypeSourceInfo()->getTypeLoc();
+  if (!RemoveStars) {
+    while (Loc.getTypeLocClass() == TypeLoc::Pointer ||
+           Loc.getTypeLocClass() == TypeLoc::Qualified)
+      Loc = Loc.getNextTypeLoc();
+  }
+  SourceRange Range(Loc.getSourceRange());
   auto Diag = diag(Range.getBegin(), "use auto when initializing with new"
                                      " to avoid duplicating the type name");
 
   // Space after 'auto' to handle cases where the '*' in the pointer type is
   // next to the identifier. This avoids changing 'int *p' into 'autop'.
-  Diag << FixItHint::CreateReplacement(Range, "auto ");
-
-  // Remove '*' from declarations using the saved star locations.
-  for (const auto &Loc : StarLocations) {
-    Diag << FixItHint::CreateReplacement(Loc, "");
-  }
+  Diag << FixItHint::CreateReplacement(Range, RemoveStars ? "auto " : "auto")
+       << StarRemovals;
 }
 
 void UseAutoCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<DeclStmt>(IteratorDeclStmtId)) {
     replaceIterators(Decl, Result.Context);
   } else if (const auto *Decl =
                  Result.Nodes.getNodeAs<DeclStmt>(DeclWithNewId)) {
     replaceNew(Decl, Result.Context);
   } else {
     llvm_unreachable("Bad Callback. No node provided.");
   }
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/modernize-use-auto.rst

 ``auto`` improving readability and maintainability.
 
 .. code-block:: c++
 
   TypeName *my_pointer = new TypeName(my_param);
 
   // becomes
 
-  auto my_pointer = new TypeName(my_param);
+  auto *my_pointer = new TypeName(my_param);
 
 The check will also replace the declaration type in multiple declarations, if
 the following conditions are satisfied:
 
 * All declared variables have the same type (i.e. all of them are pointers to
   the same type).
 * All declared variables are initialized with a ``new`` expression.
 * The types of all the new expressions are the same than the pointee of the
   declaration type.
 
 .. code-block:: c++
 
   TypeName *my_first_pointer = new TypeName, *my_second_pointer = new TypeName;
 
   // becomes
 
-  auto my_first_pointer = new TypeName, my_second_pointer = new TypeName;
+  auto *my_first_pointer = new TypeName, *my_second_pointer = new TypeName;
 
 Known Limitations
 -----------------
 * If the initializer is an explicit conversion constructor, the check will not
   replace the type specifier even though it would be safe to do so.
 
 * User-defined iterators are not handled at this time.
 
+RemoveStars option
+------------------
+If the option is set to non-zero (default is ``0``), the check will remove stars
+from the non-typedef pointer types when replacing type names with ``auto``.
+Otherwise, the check will leave stars. For example:
+
+.. code-block:: c++
+
+  TypeName *my_first_pointer = new TypeName, *my_second_pointer = new TypeName;
+
+  // RemoveStars = 0
+
+  auto *my_first_pointer = new TypeName, *my_second_pointer = new TypeName;
+
+  // RemoveStars = 1
+
+  auto my_first_pointer = new TypeName, my_second_pointer = new TypeName;

test/clang-tidy/modernize-use-auto-new-remove-stars.cpp

-// RUN: %check_clang_tidy %s modernize-use-auto %t
+// RUN: %check_clang_tidy %s modernize-use-auto %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: '1'}]}" \
+// RUN:   -- -std=c++11
 
 class MyType {};
 
 class MyDerivedType : public MyType {};
 
 // FIXME: the replacement sometimes results in two consecutive spaces after
 // the word 'auto' (due to the presence of spaces at both sides of '*').
 void auto_new() {
MyType *ptr2{new MyType};
     // '*' shouldn't be removed in case of mismatching types with multiple
     // declarations.
     MyType *j = new MyType(), *k = new MyType(), **l = new MyType*();
   }
 
   {
     // Test for typedefs.
     typedef int * int_p;
+    // CHECK-FIXES: typedef int * int_p;
 
     int_p a = new int;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
     // CHECK-FIXES: auto  a = new int;
     int_p *b = new int*;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
     // CHECK-FIXES: auto b = new int*;
 

test/clang-tidy/modernize-use-auto-new.cpp

 // RUN: %check_clang_tidy %s modernize-use-auto %t
 
 class MyType {};
 
 class MyDerivedType : public MyType {};
 
 // FIXME: the replacement sometimes results in two consecutive spaces after
 // the word 'auto' (due to the presence of spaces at both sides of '*').
 void auto_new() {
   MyType *a_new = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
-  // CHECK-FIXES: auto a_new = new MyType();
+  // CHECK-FIXES: auto *a_new = new MyType();
 
   static MyType *a_static = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use auto when initializing with new
-  // CHECK-FIXES: static auto a_static = new MyType();
+  // CHECK-FIXES: static auto *a_static = new MyType();
 
   MyType *derived = new MyDerivedType();
 
   void *vd = new MyType();
 
   // CV-qualifier tests.
   //
   // NOTE : the form "type const" is expected here because of a deficiency in
   // TypeLoc where CV qualifiers are not considered part of the type location
   // info. That is, all that is being replaced in each case is "MyType *" and
   // not "MyType * const".
   static MyType * const d_static = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use auto when initializing with new
-  // CHECK-FIXES: static auto  const d_static = new MyType();
+  // CHECK-FIXES: static auto * const d_static = new MyType();
 
   MyType * const a_const = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
-  // CHECK-FIXES: auto  const a_const = new MyType();
+  // CHECK-FIXES: auto * const a_const = new MyType();
 
   MyType * volatile vol = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
-  // CHECK-FIXES: auto  volatile vol = new MyType();
+  // CHECK-FIXES: auto * volatile vol = new MyType();
 
   struct SType {} *stype = new SType;
 
   int (**func)(int, int) = new (int(*[5])(int,int));
 
   int *array = new int[5];
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
-  // CHECK-FIXES: auto array = new int[5];
+  // CHECK-FIXES: auto *array = new int[5];
 
   MyType *ptr(new MyType);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
-  // CHECK-FIXES: auto ptr(new MyType);
+  // CHECK-FIXES: auto *ptr(new MyType);
 
   MyType *ptr2{new MyType};
 
   {
     // Test for declaration lists.
     MyType *a = new MyType(), *b = new MyType(), *c = new MyType();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto a = new MyType(), b = new MyType(), c = new MyType();
+    // CHECK-FIXES: auto *a = new MyType(), *b = new MyType(), *c = new MyType();
 
     // Non-initialized declaration should not be transformed.
     MyType *d = new MyType(), *e;
 
     MyType **f = new MyType*(), **g = new MyType*();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto f = new MyType*(), g = new MyType*();
+    // CHECK-FIXES: auto **f = new MyType*(), **g = new MyType*();
 
     // Mismatching types in declaration lists should not be transformed.
     MyType *h = new MyType(), **i = new MyType*();
 
     // '*' shouldn't be removed in case of mismatching types with multiple
     // declarations.
     MyType *j = new MyType(), *k = new MyType(), **l = new MyType*();
   }
 
   {
     // Test for typedefs.
     typedef int * int_p;
+    // CHECK-FIXES: typedef int * int_p;
 
     int_p a = new int;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto  a = new int;
+    // CHECK-FIXES: auto a = new int;
     int_p *b = new int*;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto b = new int*;
+    // CHECK-FIXES: auto *b = new int*;
 
     // Test for typedefs in declarations lists.
     int_p c = new int, d = new int;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto  c = new int, d = new int;
+    // CHECK-FIXES: auto c = new int, d = new int;
 
     // Different types should not be transformed.
     int_p e = new int, *f = new int_p;
 
     int_p *g = new int*, *h = new int_p;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use auto when initializing with new
-    // CHECK-FIXES: auto g = new int*, h = new int_p;
+    // CHECK-FIXES: auto *g = new int*, *h = new int_p;
   }
 
   // Don't warn when 'auto' is already being used.
   auto aut = new MyType();
   auto *paut = new MyType();
   const auto *pcaut = new MyType();
 }