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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
