Why are some header excluded from clang-tidy checks?

[ClausKlein]

Why are header in include/tao/json/external excluded from clang-tidy checks?

[d-frey]

Because those are external libraries from the taoJSON POV. Each project is different and clang-tidy (i.e. the rules which you want to use) need to be adapted to your specific library's needs. External libraries need to run tests on their own.

[ClausKlein]

OK, I have seen this.

But IMHO, "a chain is as strong as its weakest link"

[d-frey]

I don't understand what you mean by that. Again, if there is a problem in the PEGTL, please address that problem in taocpp/PEGTL. If there is a problem in ryu/..., it should be taken there. If we have a specific problem in our copy, please point it our specifically. Just blindly applying tools and reporting what those tools report is only creating a lot of work for very little benefit (IMHO).

[ClausKlein]

I would expect the same QA rules for the hole code.

"A collection of high-quality C++ libraries" is the topic ...

[d-frey]

The whole code except external libraries. After all, we are also using the STL. Should we also apply our .clang-tidy checks to whatever STL we are using? That simply doesn't work.

And the library in this repository is taoJSON. Obviously, we try to select other "high-quality" libraries when we need to have external libraries, but you can not simply apply the same set of checks to all of them. There is no one-size-fits-all solution here.

Note that we spend quite a lot of time on how to use clang-tidy to help with our projects, so simply throwing a log of a run not using our .clang-tidy is not going to help. clang-tidy can generate a lot of false-positives and also contains a lot of -IMHO- questionable checks that are more of a preference of someone than an actual problem.

So, again, just because clang-tidy is throwing our a warning doesn't mean that there is a problem. Please point out specific problems that you can see in our code or if you want to add more clang-tidy checks, please explain why you think they are helpful and could improve the code's quality.

PS: We have a discord, maybe we can speed up the discussion: https://discord.gg/VQYkppcgqN

[d-frey]

I just had a look at the log you uploaded. There are hundreds of pointless warnings about the external code about

• warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]
• warning: invalid case style for variable 'camelCaseIdentifier' [readability-identifier-naming]
• warning: statement should be inside braces [readability-braces-around-statements]
  (for if(...) return; instead of if( ... ) { return; }.

This is a good example of why we can't apply one set of rules to all code including the external code. It makes no sense to make so many changes to an external library, the signal-to-noise ration that we would have to deal with in case of an update would be terrible.

[ClausKlein]

Yes, i agree.

The only point is, if it would be installed and included with system path, there would be really an external lib.
No warnings from system headers be default from clang-tidy.

If you include this in your distribution, you are responsible, or not?

[d-frey]

Throwing around generalizations like

I would expect the same QA rules for the hole code.

"A collection of high-quality C++ libraries" is the topic ...

and

If you include this in your distribution, you are responsible, or not?

is not helping. You make it sound as if we don't care about the quality - which is both wrong and not very constructive, to say the least. You have to explain how you want to improve the quality of our code.

Do you think that quality can only be achieved when everything is tested by the exact same tools/rules/...? By clang-tidy? That clang-tidy (which is a style-checker) should cleanly apply to those libraries just because they are now in our tree instead of somewhere in the system (which is not generally possible in this case)? That won't do any good and I've given several arguments for that (to which you even agreed!). If you have a counter-argument, I'm open to discuss it, but so far it seems to be more of a dogmatic believe that applying clang-tidy everywhere with the same rules would help quality.

[ColinH]

Why is there a discussion, an issue, and a pull-request for what is essentially a single topic? Anyhow, did you, with your newer version of clang-tidy, find any more issues in the actual taoJSON library code?

Clang-tidy is a tool that helps us write high quality code, but it is not the most important tool or metric, neither for achieving nor for measuring code quality. What is important to us for a library are things like an intuitive interface, few bugs, and, this is also an important metric, how much time it usually takes to fix a bug.

Given the low frequency of bugs in our libraries, and the high speed and small scope usually required to fix them, I would argue that our code quality is just fine, and, at this point, I find the implication of this not being the case based on us not applying a style checker as indiscriminately as you would expect nothing more than hand-waving.