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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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

In https://reviews.llvm.org/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;
  }



================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:445-449
+size_t UseAutoCheck::GetTypeNameLength(const SourceRange &SR,
+                                       const ASTContext &Context) {
+  const StringRef S = tooling::fixit::getText(SR, Context);
+  return std::count_if(S.begin(), S.end(), SpacePredicate);
+}
----------------
zinovy.nis wrote:
> alexfh wrote:
> > ```
> > 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); });
> > }
> > ```
> `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`.
> 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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45927