[clang-tidy] [modernize-use-auto] Add a threshold for minimal type name length to be replaced with 'auto'

[zinovy.nis]

Summary

The threshold option is 'MinTypeNameLength' with default value '5'.
With MinTypeNameLength = 5 'int'/'bool' and 'const int'/'const bool' will not be converted to 'auto',
while 'unsigned' and 'long long int' will be.

[malcolm.parsons]

Please add to release notes.

[zinovy.nis]

• Updated ReleaseNotes.

[malcolm.parsons]

LGTM!

[hokein]

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

202

nit: document the default value.

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

290

Maybe make the default 5? Or does anyone really want to replace int/long/char/bool/... with auto?

[lebedev.ri]

clang-tidy/modernize/UseAutoCheck.cpp

290

That might be a bit surprising behavioral change..
At least it should be spelled out in the release notes.
(my 5 cent: defaulting to 0 would be best)

[zinovy.nis]

clang-tidy/modernize/UseAutoCheck.cpp

290

Maybe we can somehow mention the current option value in a warning message?

[lebedev.ri]

clang-tidy/modernize/UseAutoCheck.cpp

290

Sure, can be done, but that will only be seen when it does fire, not when it suddenly stops firing...

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

290

Maybe we can somehow mention the current option value in a warning message?

We don't do that for options of other checks (and for the other option here). I don't think this case is substantially different.

290

That might be a bit surprising behavioral change..

For many it will be a welcome change ;)

At least it should be spelled out in the release notes.

No objections here.

(my 5 cent: defaulting to 0 would be best)

You see it, 5 is a better default, otherwise you'd say "0 cent" ;)

On a serious note, the style guides I'm more or less familiar with recommend the use of auto for "long/cluttery type names" [1], "if and only if it makes the code more readable or easier to maintain" [2], or to "save writing a longish, hard-to-remember type that the compiler already knows but a programmer could get wrong" [3]. None of these guidelines seem to endorse the use of auto instead of int, bool or the like.

From my perspective, the default value of 5 would cover a larger fraction of real-world use cases.

[1] https://google.github.io/styleguide/cppguide.html#auto
[2] http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
[3] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es11-use-auto-to-avoid-redundant-repetition-of-type-names

420–421

Consider using tooling::fixit::getText(Loc, Context) instead.

[zinovy.nis]

clang-tidy/modernize/UseAutoCheck.cpp

290

Or even 7 to ignore 'double' too.

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

290

I'd go with 5, which is super-easy to explain: it's the smallest value that will always lead to shorter code. 7 doesn't have this advantage.

[zinovy.nis]

• Default value is 5.
• Switched to 'tooling::fixit::getText'.
• Updated rst docs.

[alexfh]

Looks good. Thank you!

[lebedev.ri]

This change had two different problems.
Please watch the bots?

[zinovy.nis]

Roman, I see you've fixed them. Thanks a lot!
I did not face with these errors on MSVS'201 so had no chance to fix early.

ср, 11 апр. 2018 г. в 0:02, Roman Lebedev via Phabricator <
[e-mail]>:

lebedev.ri added a comment.

This change had two different problems.
Please watch the bots?

Repository:

rL LLVM

https://reviews.llvm.org/D45405

[alexfh]

In D45405#1063890, @zinovy.nis wrote:

Roman, I see you've fixed them. Thanks a lot!
I did not face with these errors on MSVS'201 so had no chance to fix early.

No stress, but as Roman said, please watch the bots after committing a patch: http://lab.llvm.org:8011/console.

ср, 11 апр. 2018 г. в 0:02, Roman Lebedev via Phabricator <
[e-mail]>:

lebedev.ri added a comment.

This change had two different problems.
Please watch the bots?

[zinovy.nis]

The build was broken by someone else's commit then. From my side there was
a warning only I fixed immediately.

ср, 11 апр. 2018 г. в 16:26, Alexander Kornienko via Phabricator <
[e-mail]>:

alexfh added a comment.

In https://reviews.llvm.org/D45405#1063890, @zinovy.nis wrote:

Roman, I see you've fixed them. Thanks a lot!
I did not face with these errors on MSVS'201 so had no chance to fix

early.

No stress, but as Roman said, please watch the bots after committing a
patch: http://lab.llvm.org:8011/console.

ср, 11 апр. 2018 г. в 0:02, Roman Lebedev via Phabricator <
[e-mail]>:

lebedev.ri added a comment.

This change had two different problems.
Please watch the bots?

Repository:

rL LLVM

https://reviews.llvm.org/D45405

clang-tidy/modernize/UseAutoCheck.h

public:
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void replaceIterators(const DeclStmt *D, ASTContext *Context);
   void replaceExpr(const DeclStmt *D, ASTContext *Context,
                    llvm::function_ref<QualType(const Expr *)> GetType,
                    StringRef Message);
 
+  const unsigned int MinTypeNameLength;
   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(
                                hasType(qualType(hasDescendant(autoType()))))))),
       anyOf(makeIteratorDeclMatcher(), makeDeclWithNewMatcher(),
             makeDeclWithCastMatcher(), makeDeclWithTemplateCastMatcher()));
 }
 
 } // namespace
 
 UseAutoCheck::UseAutoCheck(StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      RemoveStars(Options.get("RemoveStars", 0)) {}
+    : ClangTidyCheck(Name, Context), RemoveStars(Options.get("RemoveStars", 0)),
+      MinTypeNameLength(Options.get("MinTypeNameLength", 0)) {}
 
 void UseAutoCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "MinTypeNameLength", MinTypeNameLength);
   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(makeCombinedMatcher(), this);
while (Loc.getTypeLocClass() == TypeLoc::Pointer ||
       Loc = Loc.getNextTypeLoc();
   }
   while (Loc.getTypeLocClass() == TypeLoc::LValueReference ||
          Loc.getTypeLocClass() == TypeLoc::RValueReference ||
          Loc.getTypeLocClass() == TypeLoc::Qualified) {
     Loc = Loc.getNextTypeLoc();
   }
   SourceRange Range(Loc.getSourceRange());
+
+  if (MinTypeNameLength != 0 &&
+      Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                           Context->getSourceManager(), Context->getLangOpts())
+              .size() < 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;

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

.. code-block:: c++
 
   // 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;
+
+.. option:: MinTypeNameLength
+
+   If the option is set to any positive non-zero integer, the check will
+   neither warn nor fix type names having a length less than the option value.
+   The option affects expressions only, not iterators.
+
+.. code-block:: c++
+
+  // MinTypeNameLength = 0
+
+  int a = static_cast<int>(foo());            // ---> auto a = ...
+  bool b = new bool;                          // ---> auto b = ...
+  unsigned c = static_cast<unsigned>(foo());  // ---> auto c = ...
+
+  // MinTypeNameLength = 8
+
+  int a = static_cast<int>(foo());            // ---> int  a = ...
+  bool b = new bool;                          // ---> bool b = ...
+  unsigned c = static_cast<unsigned>(foo());  // ---> auto 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
+
+extern int foo();
+
+using VeryVeryVeryLongTypeName = int;
+
+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());
+
+  return 1;
+}
+