Eugene.Zelenko 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 {};
----------------
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.
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
