Improve code base using clang-tidy #140
Comments
The only single warning I get is the one from #139 Notice that you can't ever guarantee it will be clang-tidy warning-free. If the user sets readability-identifier-naming options there is always going to be warnings for some specific naming style. I happen to be lucky in using the same style than trompeloeil. Even forcing the use of snack_case in Warnings in |
|
Trompeloeil version: master (@048f24e61ec52aad3f978dabc54b3f19a17ec855) File
|
|
For some reason, it never occurred to me that there's an obvious difference between warnings that are about library internals, and things that leak into user code via macros. Very valuable observation there. Thanks. One thing that keeps popping into my mind about suppressing warnings is Marshal Clow's C++Now presentation from 2017: https://www.youtube.com/watch?v=65IbEUX3QZM As in life in general, one has to choose which fights are worth fighting. I guess that what this issue boils down to is making that choice (damned!) User observable is stupendously high on that list. |
|
Latest master. This code run under clang 8.0.0 with |
|
That is because clang-tidy correctly identifies the signature of struct to_mock
{
#if 1
virtual void func1(const std::function<void(int)>&) = 0;
#else
virtual void func1(int) = 0;
#endif
};clang-tidy has no complaints. |
|
Are you saying there is something wrong with to_mock and it should be changed anyway? |
|
I'm saying that |
|
So you think in this case would be justified? |
|
I think you may have a reason to file a bug report with clang-tidy. Look here: #include <functional>
struct S
{
virtual void func(std::function<void(int)>) = 0;
};
struct SV : S
{
void func(std::function<void(int)>) override {};
};
struct SS
{
void func(std::function<void(int)>) {};
};Clang tidy gives the warning for |
|
The error message says This doesn't generate the warning: since It can't say anything about Not really sure why it doesn't complain about |
|
FWIW this is the expanded form of my original sample: clang-tidy is complaining about This patch is already enough to remove the clang-tidy warnings: |
|
The sad thing is that these functions don't have to be implemented at all. They're only ever used on I'll try to figure out some other way of doing this. Adding another work around is madness (although I fear madness may be the only route available.) |
|
FWIW I do find these tools useful. They do find real improvement situations and automate stuff that would otherwise create discussions in PRs that I would rather avoid. When I find such an issue I do try to write it in a different way to please the tool, I do w̶a̶s̶t̶e̶ spend time pleasing tools. Sometimes I end up with solutions that are actually better than the original. Most of the time they are neither better or worse, just different. But I refuse to commit anything that makes the code worse (by any metric of worse, including "a little bit more difficult to understand") just to please the tool. In such a case I just use the annotation. doctest says it "Doesn't produce any warnings even on the most aggressive warning levels for MSVC/GCC/Clang". But it does so by using a nice set of helpers to just make the compilers shut-up ( |
|
I generally agree, but suppressing a diagnostic is by definition hiding a bug. The question is if its a bug in your code or in the tool emitting the diagnostic, and it is never ever a code quality improvement. However, you may want to have a look at #145. Silencing the clang warning feels less harmful than suppressing a diagnostic caused by trying to get rid of the warning. |
|
clang-tidy 9.0.0 introduces a funny one: https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-trailing-return-type.html Every I have also created https://bugs.llvm.org/show_bug.cgi?id=43980. |
|
This one's harmless and won't clutter code. I actually quite like trailing return type. I think the reason Trompeloeil isn't written with it is just that I hadn't quite gotten used to the idea at the time. But you're raising the interesting question about which diagnostics to bother with, and which ones to ignore. Trying to satisfy everything leads to madness (like clang's warning about C++11/14/17/2a code not being compatible with C++98, and if you stoop down to writing C++98 code, you get warnings about not using nullptr and other "modern" constructs.) +1 for the llvm report. I'm guessing it'll be ignored, but it's good to raise the issue. |
Trompeloeil may be further improved through the use of
clang-tidyand its library of checks.To start this process, I am conducting a brief survey of Trompeloeil and
clang-tidyusers to gaugethe number of warnings
clang-tidyis reporting ininclude/trompeloeil.hpp.This discussion will help inform the strategy for resolving in-production
clang-tidywarningsthat may be distracting attention from production code issues.
One example issue may be found in #139,
line 4334:
misc-non-private-member-variables-in-classes.Suggested comment response format:
v34ormaster).clang-tidyused.clang-tidy.clang-tidy.Responses are requested by 31 May 2019, but planning is likely to be undertaken during the Request for Comments (RFC) period.
The text was updated successfully, but these errors were encountered: