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)) {}
----------------
Post by Roman Lebedev via Phabricator via cfe-commitsPost by Zinovy Nis via Phabricator via cfe-commitsPost by Roman Lebedev via Phabricator via cfe-commitsPost by Alexander Kornienko via Phabricator via cfe-commitsMaybe 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)) {}
----------------
Post by Roman Lebedev via Phabricator via cfe-commitsPost by Zinovy Nis via Phabricator via cfe-commitsPost by Roman Lebedev via Phabricator via cfe-commitsPost by Alexander Kornienko via Phabricator via cfe-commitsPost by Alexander Kornienko via Phabricator via cfe-commitsMaybe 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 ;)
Post by Roman Lebedev via Phabricator via cfe-commitsAt least it should be spelled out in the release notes.
No objections here.
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