clang-tidy and CppCoreGuidelines

Clang Frontend
1

Hello everybody,

i am currently trying to get an overview on the implementation status of the CppCoreGuidelines in all clang tools and especially clang-tidy.

All rules with a section on enforcement are collected here (GitHub - JonasToth/CppCoreGuidelinesTooling: Overview of tooling support for the CppCoreGuidelines), and if possible there is the checkname for clang-tidy etc. mentioned.

The implementation status in short:

number of rules: 286
number of enforced rules: 123
number of rules without enforcement: 163

The reason I am writing this here: Most of the checks that would enforce the guideline are not in the `cppcoreguidelines-*`-list.

It would be very nice, if there is an easy way to enable all checks for the guideline, but adding ~100 aliases in clang-tidy is infeasable and since many rules would apply to other guidelines as well it isn't scalable, too. Such a change would mean, that sections like `readability, modernize, performance, ...` still exist standalone as well as especially for a guideline implemented checks like `cppcoreguideline-owning-memory`. But when running clang-tidy to enforce a guideline, clang-tidy would directly activate all checks necessary to do so, even if there is no alias in the related module.

I would like to propose/discuss the possibility to have built-in configurations in clang-tidy, that would activate and configure all related checks. Such a feature would benefit other guidelines (High Integrity C++, CERT) and reduce duplication.

A quick way to implement this: custom configuration files directly shipped with clang-tidy.

A discussion on that issue would be very nice! I am willing to spent some time on an implementation for that issue as well.

All the best, Jonas

2

Hi!

3

Hopefully i won't hijack this thread :slight_smile:
Copying my related problem rant from https://reviews.llvm.org/D38171 here,
since this is basically the same problem as far as i can see:

On a somewhat related note, since this is already talking about aliases

I feel like the current handling of the clang-tidy check aliases needs
adjusting.
If one enables all the checks (`Checks: '*'`), and then disables some of them
on check-by-check basis, if the disabled check has aliases
(`cppcoreguidelines-pro-type-vararg` vs `hicpp-vararg`, `hicpp-no-array-decay`
vs `cppcoreguidelines-pro-bounds-array-to-pointer-decay` and so on)
each of the aliases must be explicitly be disabled too.

This is rather inconvenient.

If that is intentional, perhaps there could be a config option to either disable
creation/registration of the check aliases altogether, or an option to threat
aliases as a hard link, so anything happening to any of the aliases/base check
would happen to all of them.

(Also, if the check has parameters, and one specifies different
parameters for the base check, and the aliases, what happens?)

A mail [0] posted by @JonasToth is about the similar problem, i think
we can continue there.

I *think* he has a point, a some better way to setup aliases, without actually
creating aliased checks is the solution IMO. This way, there won't be any
need to disable all the aliases when completely disabling the checks, and
there won't be multiple config options for the each alias. The legacy support
is the question though.

[0] (https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html)

TLDR: if the check aliases are gone and no more, and some alternative way to
enable group of checks that implement part of some guidelines exists,
then i believe all of my problem points would be solved too.

Yes, i understand that legacy configs interoperability will need some though.

Roman.

4

I went through this process for the High Integrity Module, it was very time consuming and annoying.
When introducing an alias you must create a new documentation, setup auto redirect in docs, ensure all links work and update the checker list. This took me for ~30 aliases a week almost fulltime (including figuring out what can be aliased and so on). This will create a lot of codereview noise as well!

A related issue is, that there are currently no status pages for the guidelines. To keep track of all these things is hard and more important, time consuming.

My approach would be:

  • a status page (similar to C++17-Support page)
  • a ‘built-in’ yaml-Config file (.clang-tidy-cppcoreguidelines, .clang-tidy-cert, …)
  • multiple names for a check

Both the status page and configuration must be kept in sync, and the status page would be the documentation, that links the check options/capabilities and the actual guideline.

At the moment I can only recall readability-function-size, readability-identifier-naming and maybe modernize-use-auto that would be candidates with conflicting configuration. Others might exist, but most checks aren’t configurable.

Having builtin config-files would allow to resolve conflicts between coding standards, currently the user won’t even notice that one is an alias to another/where the alias comes from.

# say we want the coreguidelines and cert
clang-tidy -get-config cppcoreguidelines # create .clang-tidy-core-guidelines
clang-tidy -get-config cert # create .clang-tidy-cert
diff .clang-tidy-* # see the differences, do manual work

Having a workflow like that, would integrate with the current implementation of config files and would be convenient for the users, since there are no flaky reports when activating too many aliases to the same checker (e.g. modernize-use-auto would be pretty much in every guideline with possibly different configuration).

We would just provide multiple names for a check where currently only one is allowed.

What i am thinking of is going into this direction, its not a formal proposal :slight_smile:

5

To clarify what I would like to propose:

Such a change would mean, that sections like readability, modernize, performance, ... still exist standalone as well as especially for a guideline implemented checks like cppcoreguideline-owning-memory. But when running clang-tidy to enforce a guideline, clang-tidy would directly activate all checks necessary to do so, even if there is no alias in the related module.

I think there should be 2 parallel things: ‘checks’ and ‘guidelines’.

A ‘guideline’ is a collection of different checks and can combine different domains.
For example, a ‘cppcoreguidelines’-guideline consists of modernize-use-auto,cppcoreguidelines-owning-memory,.... Therefor i think a ‘guideline’ is equivalent to a custom put together configuration file.

‘checks’ on the other hand remain the same, the actual code to analyze, diagnose and maybe fix code.

For the user the interface would be like this:
clang-tidy -checks=cppcoreguidelines-* # the rest = enable all checks that are directly implemented for the cppcoreguidelines (e.g. owning-memory)
clang-tidy -guidelines=cppcoreguideline # the rest = enable all checks that are necessary to enforce the cppcoreguidelines (e.g. use-auto and owning-memory)
clang-tidy -guidelines=hicpp,cert # the rest = enable everything to ensure cert and hicpp compliance

Given, that there are currently almost no aliases in clang-5 (only some of the hicpp-* aliases land there) we could just ignore the existing aliases and treat them like normal checks. There shouldn’t be many clashes with ‘guidelines’ activating them.

The benefits are still:

  • Aliases are ugly to handle from our side
  • the source of warnings is not too obvious and it might occur that the warnings are once reported as hicpp-use-auto and modernize-use-auto
  • bad documentation, there is no high level view (50% of the guideline is implemented, see here for general advice and so on) like a status page
  • do not scale well. Having to maintain and keep track of potentially hundreds of aliases is harder for the current form

One current approximation for that approach would be custom delivered configuration files, that would be activated by the -guidelines - flag.
I would be happy to deliver these for hicpp and cppcoreguidelines, since i have done these pretty much already to get an overview of whats already implemented.