Fix doubled whitespace in modernize-use-auto fixit

[malcolm.parsons]

Summary

Check whether the character following the type is whitespace to avoid
doubled whitespace in fixit.

[Prazek]

LGTM

[aaron.ballman]

clang-tidy/modernize/UseAutoCheck.cpp

378

Oye, this is deceptively expensive because you now have to go back to the actual source file for this information. That source file may live on a network share somewhere, for instance.

Can you use Token::hasLeadingSpace() instead?

Also, doesn't this still need to care about the RemoveStars option?

[malcolm.parsons]

Only insert whitespace when removing stars

clang-tidy/modernize/UseAutoCheck.cpp

378

Where would I get a Token from?

[aaron.ballman]

clang-tidy/modernize/UseAutoCheck.cpp

378

Hrm, might not be as trivial as I was hoping (I thought we had a way to go from a SourceLocation back to a Token, but I'm not seeing one off-hand). Regardless, I worry about the expense of going all the way back to the source for this.

@alexfh -- should this functionality be a part of a more general "we've made a fixit, now make it not look ugly?" pass? At least then, if we go back to the source, we can do it in a more controlled manner and hopefully get some performance back from that.

[malcolm.parsons]

clang-tidy/modernize/UseAutoCheck.cpp

378

LineIsMarkedWithNOLINT is going to read the source anyway, so I don't see any additional expense.

[aaron.ballman]

clang-tidy/modernize/UseAutoCheck.cpp

378

The additional expense comes from many checks needing similar functionality -- generally, fixit replacements are going to require formatting modifications of some sort. It's better to handle all of those in the same place and transparently rather than have every check with a fixit manually handle formatting. Additionally, this means we can go back to the source as infrequently as possible.

[malcolm.parsons]

clang-tidy/modernize/UseAutoCheck.cpp

378

I ran strace -c on clang-tidy on several real world source files that modernize-use-auto warns about before and after this change and the counts for read, write, stat, open, close, openat and fstat syscalls were all unchanged.
The expense is exactly zero.
It will still be zero when multiplied by many checks.
There is no performance issue here.
Do you have any other issues with this change?

[aaron.ballman]

clang-tidy/modernize/UseAutoCheck.cpp

378

Thank you for running strace to see what the behavior is. From what I understand, this is hard to judge because it depends on when the SourceManager elects to drop its underlying memory buffer (if at all). We definitely try to avoid going back to source when possible in Clang; I think clang-tidy should take a similar policy.

I don't think the fix you have is incorrect. I am, however, not convinced this is the best way to solve the problem because we're playing whack-a-mole with fixing replacements already, and this is one more instance of such need. @alexfh had suggested (in a different review thread about a similar need) that we solve this through a convenient bottleneck (such as the diagnostics engine) that basically runs clang-format over replaced text, handles extraneous commas, etc and I would prefer to hear if efforts are being made/planned for that approach before committing another one-off solution. The extra space included in the current behavior is not a correctness issue, so I'm not too worried about getting this fix in immediately if there's work in progress on a better approach.

[malcolm.parsons]

ping @alexfh.

[malcolm.parsons]

I'd prefer a -format option to clang-tidy.

[alexfh]

In D25406#633892, @malcolm.parsons wrote:

I'd prefer a -format option to clang-tidy.

Exactly.

clang-tidy/modernize/UseAutoCheck.h

 public:
   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);
+  void replaceNew(const DeclStmt *D, ASTContext *Context, SourceManager &SM);
 
   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

 //===--- UseAutoCheck.cpp - clang-tidy-------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseAutoCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
void UseAutoCheck::replaceIterators(const DeclStmt *D, ASTContext *Context) {
   // like function pointers. Not a concern since this action only works with
   // iterators but something to keep in mind in the future.
 
   SourceRange Range(V->getTypeSourceInfo()->getTypeLoc().getSourceRange());
   diag(Range.getBegin(), "use auto when declaring iterators")
       << FixItHint::CreateReplacement(Range, "auto");
 }
 
-void UseAutoCheck::replaceNew(const DeclStmt *D, ASTContext *Context) {
+void UseAutoCheck::replaceNew(const DeclStmt *D, ASTContext *Context,
+                              SourceManager &SM) {
   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<FixItHint> StarRemovals;
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");
 
+  SourceLocation Next =
+      Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts());
+  bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData());
+
   // 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, RemoveStars ? "auto " : "auto")
+  Diag << FixItHint::CreateReplacement(
+              Range, (Whitespace || !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);
+    replaceNew(Decl, Result.Context, *Result.SourceManager);
   } else {
     llvm_unreachable("Bad Callback. No node provided.");
   }
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

 // 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 *a_new = new MyType();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with new
   // 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();
 
   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];
void auto_new() {
 
   {
     // 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*;
 
     // 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;
   }
 
   // Don't warn when 'auto' is already being used.
   auto aut = new MyType();
   auto *paut = new MyType();
   const auto *pcaut = new MyType();
 }