[PATCH] D72217: [clang-tidy] Added readability-qualified-auto check

[Nathan James via Phabricator via cfe-commits]

njames93 marked an inline comment as done.
njames93 added a comment.

In D72217#1809887 <https://reviews.llvm.org/D72217#1809887>, @JonasToth wrote:

> In D72217#1809721 <https://reviews.llvm.org/D72217#1809721>, @njames93 wrote:
>
> > In D72217#1809685 <https://reviews.llvm.org/D72217#1809685>, @njames93 wrote:
> >
> > > In D72217#1809212 <https://reviews.llvm.org/D72217#1809212>, @Eugene.Zelenko wrote:
> > >
> > > > By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.
> > >
> > >
> > > So ran it on clang and clang-tidy just crashed, gonna debug it see whats happening
> >
> >
> > Crash fixed heres what happened when I ran it on clang/lib
> >
> > Quite a few occurances of const auto Decl which are renamed as const auto* const, or worse still a few that are redeclared as auto *const
> >
> >   /home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:623:3: warning: 'const auto IC' can be declared as 'const auto *const IC' [readability-qualified-auto]
> >     const auto IC = dyn_cast<CXXInstanceCall>(&Call);
> >     ^~~~~~~~~~~
> >     const auto *const 
> >   /home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:636:3: warning: 'const auto MethodDecl' can be declared as 'const auto *const MethodDecl' [readability-qualified-auto]
> >     const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
> >     ^~~~~~~~~~~
> >     const auto *const 
> >
> >
> > 286 files changed, without looking at header files. One file failed to build which is due to a dependant template. My guess is one call will have returned a naked pointer, and another returned an iterator, maybe I should disregard dependant templates unless it would be possible to deduce its always a pointer
>
>
> Yes, templates are usually not so nice to handle well :/
>  One unfortunate thing I am currently looking for is checking if `auto->deducedType()` is a `tempateTypeParmType` or `substTemplateTypeParmType` through matchers, I failed. But in a sense you need to check for that condition for the cases `auto` -> direct; `auto &` -> pointee; `auto *` -> pointee.
>  From my experience it helps to run it over `llvm/lib`, too as there is a lot of interesting c++ code in their. My checks broke there more often.


I managed to make some headway with templates, but I think I'll need to add some new ast matchers to get full coverage. However this patch is a little big for more AST matchers as well so that can come at a later date. Right now the behaviour is ignore template instantiations apart from function instantiations where the type is deduced as T*. using that I'm able to run across the whole of llvm and clang lib with no errors in compilation



================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146
 
+- New alias :doc:`llvm-qualified-auto
+  <clang-tidy/checks/llvm-qualified-auto>` to
----------------
Eugene.Zelenko wrote:
> njames93 wrote:
> > Eugene.Zelenko wrote:
> > > Please move alias entry into aliases section. See previous release for proper order.
> > I looked at the previous release notes and from what i can see, all alias checks are next to each other in alphabetical order, but there is no special placement of the alias checks in the release notes, they just start at an arbitrary position. 
> No, aliases are after new checks. See http://releases.llvm.org/9.0.0/tools/clang/tools/extra/docs/ReleaseNotes.html.
Ahh I see, what threw me is there is an alias in release notes in master that is just randomly thrown in the middle, guess I'll change both of them


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:402
    `hicpp-vararg <hicpp-vararg.html>`_, `cppcoreguidelines-pro-type-vararg <cppcoreguidelines-pro-type-vararg.html>`_,
+   `llvm-qualified-auto <llvm-qualified-auto.html>`_, `readability-qualified-auto <readability-qualified-auto.html>`_, "Yes"
 
----------------
JonasToth wrote:
> That line needs to land above where the other LLVM checks are.
How come, thought this section was just for alias declarations? or do llvm checks not get listed in the alias section


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72217/new/

https://reviews.llvm.org/D72217