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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:290
+    : ClangTidyCheck(Name, Context), RemoveStars(Options.get("RemoveStars", 0)),
+      MinTypeNameLength(Options.get("MinTypeNameLength", 0)) {}
 
----------------
lebedev.ri wrote:
> zinovy.nis wrote:
> > lebedev.ri wrote:
> > > alexfh wrote:
> > > > Maybe make the default 5? Or does anyone really want to replace `int/long/char/bool/...` with `auto`?
> > > 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)
> > Maybe we can somehow mention the current option value in a warning message?
> Sure, can be done, but that will only be seen when it does fire, not when it suddenly stops firing...
> 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.


================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:290
+    : ClangTidyCheck(Name, Context), RemoveStars(Options.get("RemoveStars", 0)),
+      MinTypeNameLength(Options.get("MinTypeNameLength", 0)) {}
 
----------------
alexfh wrote:
> lebedev.ri wrote:
> > zinovy.nis wrote:
> > > lebedev.ri wrote:
> > > > alexfh wrote:
> > > > > Maybe make the default 5? Or does anyone really want to replace `int/long/char/bool/...` with `auto`?
> > > > 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)
> > > Maybe we can somehow mention the current option value in a warning message?
> > Sure, can be done, but that will only be seen when it does fire, not when it suddenly stops firing...
> > 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.
> 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


================
Comment at: clang-tidy/modernize/UseAutoCheck.cpp:420-421
+  if (MinTypeNameLength != 0 &&
+      Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                           Context->getSourceManager(), Context->getLangOpts())
+              .size() < MinTypeNameLength)
----------------
Consider using tooling::fixit::getText(Loc, Context) instead.


https://reviews.llvm.org/D45405