[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Bernhard Manfred Gruber via Phabricator via cfe-commits]

bernhardmgruber added inline comments.


================
Comment at: clang-tidy/modernize/UseTrailingReturnCheck.cpp:45
+        std::string(Message) +
+            " (no FixIt provided, function argument list end is inside macro)");
+    return {};
----------------
Eugene.Zelenko wrote:
> MyDeveloperDay wrote:
> > bernhardmgruber wrote:
> > > JonasToth wrote:
> > > > I think you can ellide that extra message. Not emitting the fixit is clear already.
> > > I actually like having a reason why my check could not provide a FixIt. Especially because there are multiple reasons why this might happen.
> > @bernhardmgruber I had the same comment given to me on a review recently with regard to diag message, let me try and help you with what I though was the rational... I think the premise is something like:
> > 
> > 1) "no FixIt provided" is implied by the fact it isn't fixed
> > 2) "function type source info is missing"  doesn't tell the developer what they have to do to have it be fixed
> > 
> > sure it helps you as the checker developer but probably that isn't much use to a developer using the checker on their code and so might confuse them.
> > 
> > It also makes grepping for messages in a log file difficult because it means multiple messages coming from your checker have a different pattern (although there might be a common sub pattern)
> > 
> > For the most part where a fixit is not supplied where it should someone would create a test case which you could consume in your tests
> > 
> > To be honest as a general observation as a newbie myself, what I've noticed is that a lot of checker review comments are very similar, 
> > 
> > 
> > 
> >   - 80 characters in rst files
> >   - clang-format
> >   - alphabetic order
> >   - Comments with proper puncuation
> >   - code in comments in ``XXX``
> >   - don't overly use auto
> >   - don't use anonymous namespace functions use static functions
> >   - run it on a big C++ project
> >   - run it over all of LLVM
> >   - consistency of coding style (elide unnecessary const)
> >   - elide unnecessary braces/brackets/code/etc..
> > 
> > 
> > 
> > We really should try and write a "Writing a clang checker, and getting it though review" primer, because I really feel for these "gaints" that we ask to review all this code, they must go over the same thing and have to present the same reasons time and time again...
> > 
> > which is why If you don't mind I'd like to try to help give something back by filling in some of the reasoning gaps here to a fellow new starter
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> I would say that we should eat own dog food :-)
> 
> I'd love to see your documentation validation scripts as part of build!
> 
> We also should regularly run Clang-tidy on BuildBot. But first we must fix existing warnings and no everybody happy if such cleanups performed by outsiders.
> 
> See PR27267 for anonymous namespaces usage.
> 
> Clang-tidy has modernize-use-auto, but not check for LLVM guidelines conformance.
> 
> Braces should be checked by readability-braces-around-statements, but proper setup is needed.
> 
> Conformance to readability-else-after-return is another typical newbies problem.
Thank you @MyDeveloperDay for the list of tips and rational behind the diagnostic messages. I will check this list in the future before I send new patches. Maybe it is really a good idea to put this list somewhere!


================
Comment at: test/clang-tidy/modernize-use-trailing-return.cpp:1
+// RUN: %check_clang_tidy %s modernize-use-trailing-return %t -- -- --std=c++14
+
----------------
MyDeveloperDay wrote:
> bernhardmgruber wrote:
> > MyDeveloperDay wrote:
> > > nit: is there a reason here why you say C++14 when the code checks for C++11? 
> > Yes. The tests contain functions with deduced return types, such as `auto f();`. Those require C++14. The check itself is fine with C++11.
> I kind of half guessed it would be something like that after I hit submit,  I noticed some checks add secondary test files which test the various versions of C++, to be honest I found this useful for the checker I'm developing, especially as the checker has some slightly different behavior with C++11 to C++17, but maybe yours doesn't
> 
> to be honest i'm also fairly new here so don't know exactly the convention
> 
> examples where this is already done in other peoples checkers
> 
> modernize-deprecated-headers-cxx03.cpp
> modernize-deprecated-headers-cxx11.cpp
> 
> ```
> // RUN: %check_clang_tidy %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers -- -std=c++03 -v
> 
> // RUN: %check_clang_tidy %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers -- -std=c++11 -v
> 
> ```
> 
Thank you for the hint! I just split my tests into C++11 and C++14 versions, but then I realized that for C++14 there are only two tests, where one is even currently commented out:

```
auto f1();
//decltype(auto) f2();
```
I exclude auto as a return type in the checker and this also excludes decltype(auto). Initially, I also wanted to rewrite decltype(auto), but I have no strong opinions. Maybe this could be an option at some point. Some users might prefer something like:

```
auto f() -> decltype(auto);
```
But then we can also discuss:

```
auto f() -> const auto&;
```
I have written all this at some point, because I loved the aesthetics of having the function name at column 6 everywhere. But I believe for now it is good enough to just put concrete types to the back.


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

https://reviews.llvm.org/D56160