[clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types

[zinovy.nis]

Summary

This patch is a fix for D45405 where spaces were also considered as a part of a type name. So length("int *") was 5 instead of 3 with RemoveStars=0 or 4 with RemoveStars=1.

[malcolm.parsons]

I think spaces that will be removed should be counted - long long is 9.

OTOH, MinTypeNameLength isn't likely to be set high enough for that to matter.

[zinovy.nis]

I think spaces that will be removed should be counted - long long is 9.

I thought about it, but what about "long long int

• * * *"? Is it still 9?

I think it's a business of clang-format to remove excessive spaces, not of
clang-tidy.

вс, 22 апр. 2018 г. в 10:11, Malcolm Parsons via Phabricator <
[e-mail]>:

malcolm.parsons added a comment.

I think spaces that will be removed should be counted - long long is 9.

OTOH, MinTypeNameLength isn't likely to be set high enough for that to
matter.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45927

[zinovy.nis]

Gentle ping.

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

32

Why const char& and not just char? Moreover, these two functions can be replaced with lambdas. See below.

467–471

static size_t GetTypeNameLength(const TypeLoc &Loc, const ASTContext &Context, bool IgnoreStars) {
  const StringRef S = tooling::fixit::getText(Loc.getSourceRange(), Context);
  if (IgnoreStars)
    return llvm::count_if(S, [] (char C) { return std::isspace(C) || C == '*'; });
  return llvm::count_if(S, [] (char C) { return std::isspace(C); });
}

clang-tidy/modernize/UseAutoCheck.h

31 ↗

(On Diff #143465)

I'd make this a static function and remove the SpacePredicate field as well. See the comment below.

[zinovy.nis]

clang-tidy/modernize/UseAutoCheck.cpp

32

Agree.

467–471

IgnoreStars is initialized once in the ctor and is used widely for all the literals in the translation unit.
IMHO it's better to eliminate branches on IgnoreStars.

[alexfh]

Actually, just ignoring spaces may be not the best option.

In D45927#1074593, @zinovy.nis wrote:

I think spaces that will be removed should be counted - long long is 9.

I thought about it, but what about "long long int

• * * *"? Is it still 9?

I think, it's 13, if you choose to remove stars, and 17 otherwise. The difference is excessive spaces vs. required ones. Implementing proper logic may be involved, but we can simplify it to something like "count all non-space characters and a single space between words, but don't count spaces around punctuation":

enum CharType { Space, Alpha, Punctuation };
CharType LastChar = Space, BeforeSpace = Punctuation;
int NumChars = 0;
for (char C : S) {
  CharType NextChar = std::isalnum(C) ? Alpha : (std::isspace(C) || RemoveStars && C == '*') ? Space : Punctuation;
  if (NextChar != Space) {
    ++NumChars; // Count the non-space character.
    if (LastChar == Space && NextChar == Alpha && BeforeSpace == Alpha)
      ++NumChars; // Count a single space character between two words.
    BeforeSpace = NextChar;
  }
  LastChar = NextChar;
}

clang-tidy/modernize/UseAutoCheck.cpp

467–471

IMHO it's better to eliminate branches on IgnoreStars.

Why do you think so?

I think that spreading the implementation of a rather trivial function to one method, one data member, two functions and a constructor makes the code significantly harder to read. And you for some reason propose to avoid a single branch that doesn't seem to be on any hot path. Am I missing something?

[zinovy.nis]

I think, it's 13, if you choose to remove stars, and 17 otherwise. The difference is excessive spaces vs. required ones. Implementing proper logic may be involved, but we can simplify it to something like "count all non-space characters and a single space between words, but don't count spaces around punctuation":

Isn't it a business of clang-format to determine the number of spaces between lexemes? IMO the solutuion you've provided for GetTypeNameLength is enough. Considering punctuation here is overkill :-)

[alexfh]

In D45927#1083051, @zinovy.nis wrote:

I think, it's 13, if you choose to remove stars, and 17 otherwise. The difference is excessive spaces vs. required ones. Implementing proper logic may be involved, but we can simplify it to something like "count all non-space characters and a single space between words, but don't count spaces around punctuation":

Isn't it a business of clang-format to determine the number of spaces between lexemes? IMO the solutuion you've provided for GetTypeNameLength is enough. Considering punctuation here is overkill :-)

The algorithm in your patch makes decisions based on the assumption that there may be no spaces at all, which is definitely wrong. I'm proposing an algorithm that would at least handle things like unsigned char correctly.

[zinovy.nis]

• Applied Alexander's changes.

[alexfh]

clang-tidy/modernize-use-auto-min-type-name-length.cpp

61–83 ↗

(On Diff #144727)

These tests could be more useful, if they verified boundary values of the MinTypeNameLength, e.g. that long int is replaced with auto when the option is set to 8 and it stays long intwith the option set to 9.

Please also add tests with template typenames: e.g. the lenght of T < int > should be considered 6 and the length of T < const int > is 12. I guess, the algorithm I proposed will be incorrect for pointer type arguments of templates (the length of T<int*> should be 7 regardless of RemoveStars), but this can be fixed by conditionally (on RemoveStars) trimming *s from the right of the type name and then treating them as punctuation. So instead of

for (const unsigned char C : tooling::fixit::getText(SR, Context)) {
  const CharType NextChar =
      std::isalnum(C)
          ? Alpha
          : (std::isspace(C) || (!RemoveStars && C == '*')) ? Space
                                                            : Punctuation;

you could use something similar to

StringRef Text = tooling::fixit::getText(SR, Context);
if (RemoveStars)
  Text = Text.rtrim(" \t\v\n\r*");
for (const unsigned char C : Text) {
  const CharType NextChar =
      std::isalnum(C) ? Alpha : std::isspace(C) ? Space : Punctuation;

[zinovy.nis]

clang-tidy/modernize-use-auto-min-type-name-length.cpp

61–83 ↗

(On Diff #144727)

These tests could be more useful, if they verified boundary values of the MinTypeNameLength, e.g. that long int is replaced with auto when the option is set to 8 and it stays long intwith the option set to 9.

int-test is just for that :-)

int a = static_cast<int>(foo());                 // ---> int  a = ...

[zinovy.nis]

clang-tidy/modernize-use-auto-min-type-name-length.cpp

61–83 ↗

(On Diff #144727)

I measured lengths for template cases:

S=std::string *   len=12
S=std::vector<std::string> *   len=25
S=std::vector<const std::string> *   len=31
S=std::string        *   len=12
S=std::vector<std::string>       *   len=25
S=std::vector<const std::string>    *   len=31

[zinovy.nis]

clang-tidy/modernize-use-auto-min-type-name-length.cpp

61–83 ↗

(On Diff #144727)

RemoveStars==1 here.

[rsmith]

modernize/UseAutoCheck.cpp

40 ↗

(On Diff #144727)

isspace here is wrong: it's a locale-sensitive check. Clang assumes UTF-8 throughout; you can use isWhitespace from clang/Basic/CharInfo.h to test if the character should be treated as whitespace.

[alexfh]

clang-tidy/modernize-use-auto-min-type-name-length.cpp

61–83 ↗

(On Diff #144727)

S=std::string * len=12

With RemoveStars this should be 11?

S=std::vector<std::string> * len=25

24?

S=std::vector<const std::string> * len=31

30?

The point of my comment was that stars should be treated specially only at the end of the type, not inside template parameters, e.g. T<int****> should have length 10 regardless of RemoveStars. All your examples are with trailing stars.

61–83 ↗

(On Diff #144727)

int-test is just for that :-)

Nope, the point is to verify that the length of multi-token type names is calculated correctly. int is a single token.

[zinovy.nis]

• Fix for templated type names. Thanks AlexanderK for pointing.

[zinovy.nis]

modernize/UseAutoCheck.cpp

40 ↗

(On Diff #144727)

Thanks!

[zinovy.nis]

Alexander, can you please have a look at the latest patch? Thanks.

[alexfh]

LG

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/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/CharInfo.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 using namespace clang::ast_matchers::internal;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 namespace {
 
 const char IteratorDeclStmtId[] = "iterator_decl";
 const char DeclWithNewId[] = "decl_new";
 const char DeclWithCastId[] = "decl_cast";
 const char DeclWithTemplateCastId[] = "decl_template";
 
+size_t GetTypeNameLength(bool RemoveStars, StringRef Text) {
+  enum CharType { Space, Alpha, Punctuation };
+  CharType LastChar = Space, BeforeSpace = Punctuation;
+  size_t NumChars = 0;
+  int TemplateTypenameCntr = 0;
+  for (const unsigned char C : Text) {
+    if (C == '<')
+      ++TemplateTypenameCntr;
+    else if (C == '>')
+      --TemplateTypenameCntr;
+    const CharType NextChar =
+        isAlphanumeric(C)
+            ? Alpha
+            : (isWhitespace(C) ||
+               (!RemoveStars && TemplateTypenameCntr == 0 && C == '*'))
+                  ? Space
+                  : Punctuation;
+    if (NextChar != Space) {
+      ++NumChars; // Count the non-space character.
+      if (LastChar == Space && NextChar == Alpha && BeforeSpace == Alpha)
+        ++NumChars; // Count a single space character between two words.
+      BeforeSpace = NextChar;
+    }
+    LastChar = NextChar;
+  }
+  return NumChars;
+}
+
 /// \brief Matches variable declarations that have explicit initializers that
 /// are not initializer lists.
 ///
 /// Given
 /// \code
 ///   iterator I = Container.begin();
 ///   MyType A(42);
 ///   MyType B{2};
void UseAutoCheck::replaceExpr(
   while (Loc.getTypeLocClass() == TypeLoc::LValueReference ||
          Loc.getTypeLocClass() == TypeLoc::RValueReference ||
          Loc.getTypeLocClass() == TypeLoc::Qualified) {
     Loc = Loc.getNextTypeLoc();
   }
   SourceRange Range(Loc.getSourceRange());
 
   if (MinTypeNameLength != 0 &&
-      tooling::fixit::getText(Loc.getSourceRange(), FirstDecl->getASTContext())
-              .size() < MinTypeNameLength)
+      GetTypeNameLength(RemoveStars,
+                        tooling::fixit::getText(Loc.getSourceRange(),
+                                                FirstDecl->getASTContext())) <
+          MinTypeNameLength)
     return;
 
   auto Diag = diag(Range.getBegin(), Message);
 
   // Space after 'auto' to handle cases where the '*' in the pointer type is
   // next to the identifier. This avoids changing 'int *p' into 'autop'.
   // FIXME: This doesn't work for function pointers because the variable name
   // is inside the type.
   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)) {
     replaceExpr(Decl, Result.Context,
                 [](const Expr *Expr) { return Expr->getType(); },
                 "use auto when initializing with new to avoid "
                 "duplicating the type name");
   } else if (const auto *Decl =
                  Result.Nodes.getNodeAs<DeclStmt>(DeclWithCastId)) {
     replaceExpr(
         Decl, Result.Context,

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

 Options
 -------
 
 .. option:: MinTypeNameLength
 
    If the option is set to non-zero (default `5`), the check will ignore type
    names having a length less than the option value. The option affects
    expressions only, not iterators.
+   Spaces between multi-lexeme type names (``long int``) are considered as one.
+   If ``RemoveStars`` option (see below) is set to non-zero, then ``*s`` in
+   the type are also counted as a part of the type name.
 
 .. code-block:: c++
 
-  // MinTypeNameLength = 0
+  // MinTypeNameLength = 0, RemoveStars=0
 
   int a = static_cast<int>(foo());            // ---> auto a = ...
-  bool b = new bool;                          // ---> auto b = ...
+  // length(bool *) = 4
+  bool *b = new bool;                         // ---> auto *b = ...
   unsigned c = static_cast<unsigned>(foo());  // ---> auto c = ...
 
-  // MinTypeNameLength = 8
+  // MinTypeNameLength = 5, RemoveStars=0
 
-  int a = static_cast<int>(foo());            // ---> int  a = ...
-  bool b = new bool;                          // ---> bool b = ...
-  unsigned c = static_cast<unsigned>(foo());  // ---> auto c = ...
+  int a = static_cast<int>(foo());                 // ---> int  a = ...
+  bool b = static_cast<bool>(foo());               // ---> bool b = ...
+  bool *pb = static_cast<bool*>(foo());            // ---> bool *pb = ...
+  unsigned c = static_cast<unsigned>(foo());       // ---> auto c = ...
+  // length(long <on-or-more-spaces> int) = 8
+  long int d = static_cast<long int>(foo());       // ---> auto d = ...
+
+  // MinTypeNameLength = 5, RemoveStars=1
+
+  int a = static_cast<int>(foo());                 // ---> int  a = ...
+  // length(int * * ) = 5
+  int **pa = static_cast<int**>(foo());            // ---> auto pa = ...
+  bool b = static_cast<bool>(foo());               // ---> bool b = ...
+  bool *pb = static_cast<bool*>(foo());            // ---> auto pb = ...
+  unsigned c = static_cast<unsigned>(foo());       // ---> auto c = ...
+  long int d = static_cast<long int>(foo());       // ---> auto d = ...
 
 .. option:: RemoveStars
 
    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++

test/clang-tidy/modernize-use-auto-min-type-name-length.cpp

-// RUN: %check_clang_tidy %s modernize-use-auto %t -- \
-// RUN:   -config="{CheckOptions: [{key: modernize-use-auto.MinTypeNameLength, value: '5'}]}" \
-// RUN:   -- -std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=0-0 %s modernize-use-auto %t  -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 0}, {key: modernize-use-auto.MinTypeNameLength, value: 0}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=0-8 %s modernize-use-auto %t  -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 0}, {key: modernize-use-auto.MinTypeNameLength, value: 8}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=1-0 %s modernize-use-auto %t  -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 1}, {key: modernize-use-auto.MinTypeNameLength, value: 0}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=1-8 %s modernize-use-auto %t  -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 1}, {key: modernize-use-auto.MinTypeNameLength, value: 8}]}" -- --std=c++11 -frtti
+
+template <class T> extern T foo();
+template <class T> struct P {  explicit P(T t) : t_(t) {}  T t_;};
+template <class T> P<T> *foo_ptr();
+template <class T> P<T> &foo_ref();
 
-extern int foo();
+int bar() {
+  {
+    // Lenth(long) = 4
+    long i = static_cast<long>(foo<long>());
+    // CHECK-FIXES-0-0: auto i = {{.*}}
+    // CHECK-FIXES-0-8: long i = {{.*}}
+    // CHECK-FIXES-1-0: auto  i = {{.*}}
+    // CHECK-FIXES-1-8: long i = {{.*}}
+    const long ci = static_cast<long>(foo<const long>());
+    // CHECK-FIXES-0-0: auto ci = {{.*}}
+    // CHECK-FIXES-0-8: long ci = {{.*}}
+    // CHECK-FIXES-1-0: auto  ci = {{.*}}
+    // CHECK-FIXES-1-8: long ci = {{.*}}
+    long *pi = static_cast<long *>(foo<long *>());
+    // CHECK-FIXES-0-0: auto *pi = {{.*}}
+    // CHECK-FIXES-0-8: long *pi = {{.*}}
+    // CHECK-FIXES-1-0: auto pi = {{.*}}
+    // CHECK-FIXES-1-8: long *pi = {{.*}}
+
+    // Length(long       *) is still 5
+    long      *     pi2 = static_cast<long *>(foo<long *>());
+    // CHECK-FIXES-0-0: auto      *     pi2 = {{.*}}
+    // CHECK-FIXES-0-8: long      *     pi2 = {{.*}}
+    // CHECK-FIXES-1-0: auto      pi2 = {{.*}}
+    // CHECK-FIXES-1-8: long      *     pi2 = {{.*}}
+
+    // Length(long **) = 6
+    long **ppi = static_cast<long **>(foo<long **>());
+    // CHECK-FIXES-0-0: auto **ppi = {{.*}}
+    // CHECK-FIXES-0-8: long **ppi = {{.*}}
+    // CHECK-FIXES-1-0: auto ppi = {{.*}}
+    // CHECK-FIXES-1-8: long **ppi = {{.*}}
+  }
 
-using VeryVeryVeryLongTypeName = int;
+  {
+    // Lenth(long int) = 4 + 1 + 3 = 8
+    // Lenth(long        int) is still 8
+    long int i = static_cast<long int>(foo<long int>());
+    // CHECK-FIXES-0-0: auto i = {{.*}}
+    // CHECK-FIXES-0-8: auto i = {{.*}}
+    // CHECK-FIXES-1-0: auto  i = {{.*}}
+    // CHECK-FIXES-1-8: auto  i = {{.*}}
+
+    long int *pi = static_cast<long int *>(foo<long int *>());
+    // CHECK-FIXES-0-0: auto *pi = {{.*}}
+    // CHECK-FIXES-0-8: auto *pi = {{.*}}
+    // CHECK-FIXES-1-0: auto pi = {{.*}}
+    // CHECK-FIXES-1-8: auto pi = {{.*}}
+  }
 
-int bar() {
-  int a = static_cast<VeryVeryVeryLongTypeName>(foo());
-  // strlen('int') = 4 <  5, so skip it,
-  // even strlen('VeryVeryVeryLongTypeName') > 5.
-
-  unsigned b = static_cast<unsigned>(foo());
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
-  // CHECK-FIXES: auto b = static_cast<unsigned>(foo());
-
-  bool c = static_cast<bool>(foo());
-  // strlen('bool') = 4 <  5, so skip it.
-
-  const bool c1 = static_cast<const bool>(foo());
-  // strlen('bool') = 4 <  5, so skip it, even there's a 'const'.
-
-  unsigned long long ull = static_cast<unsigned long long>(foo());
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
-  // CHECK-FIXES: auto ull = static_cast<unsigned long long>(foo());
+  // Templates
+  {
+    // Length(P<long>) = 7
+    P<long>& i = static_cast<P<long>&>(foo_ref<long>());
+    // CHECK-FIXES-0-0: auto& i = {{.*}}
+    // CHECK-FIXES-0-8: P<long>& i = {{.*}}
+    // CHECK-FIXES-1-0: auto & i = {{.*}}
+    // CHECK-FIXES-1-8: P<long>& i = {{.*}}
+
+    // Length(P<long*>) = 8
+    P<long*>& pi = static_cast<P<long*> &>(foo_ref<long*>());
+    // CHECK-FIXES-0-0: auto& pi = {{.*}}
+    // CHECK-FIXES-0-8: auto& pi = {{.*}}
+    // CHECK-FIXES-1-0: auto & pi = {{.*}}
+    // CHECK-FIXES-1-8: auto & pi = {{.*}}
+
+    P<long>* pi2 = static_cast<P<long>*>(foo_ptr<long>());
+    // CHECK-FIXES-0-0: auto* pi2 = {{.*}}
+    // CHECK-FIXES-0-8: P<long>* pi2 = {{.*}}
+    // CHECK-FIXES-1-0: auto  pi2 = {{.*}}
+    // CHECK-FIXES-1-8: auto  pi2 = {{.*}}
+  }
 
   return 1;
 }
-