clang-tidy and CppCoreGuidelines

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

clang-tidy and CppCoreGuidelines

Eric Fiselier via cfe-dev
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
(https://github.com/JonasToth/CppCoreGuidelinesTooling), 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy and CppCoreGuidelines

Eric Fiselier via cfe-dev
Hi!

On 24 September 2017 at 18:53, Jonas Toth via cfe-dev <[hidden email]> wrote:
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 (https://github.com/JonasToth/CppCoreGuidelinesTooling), 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.

Why do you think it is not scalable or infeasible? If we introduce built-in configurations we still need to maintain the "aliases" but in different files. I see only one advantage here, when different configurations are required by different aliases. But I do not know how frequent such checks are.

Regards,
Gábor
 
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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy and CppCoreGuidelines

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
Hopefully i won't hijack this thread :)
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.



On Sun, Sep 24, 2017 at 7:53 PM, Jonas Toth via cfe-dev
<[hidden email]> wrote:

> 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
> (https://github.com/JonasToth/CppCoreGuidelinesTooling), 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
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy and CppCoreGuidelines

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev
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 :)

Am 25.09.2017 um 14:33 schrieb Gábor Horváth:
Hi!

On 24 September 2017 at 18:53, Jonas Toth via cfe-dev <[hidden email]> wrote:
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 (https://github.com/JonasToth/CppCoreGuidelinesTooling), 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.

Why do you think it is not scalable or infeasible? If we introduce built-in configurations we still need to maintain the "aliases" but in different files. I see only one advantage here, when different configurations are required by different aliases. But I do not know how frequent such checks are.

Regards,
Gábor
 
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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy and CppCoreGuidelines

Eric Fiselier via cfe-dev
In reply to this post by Eric Fiselier via cfe-dev

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.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev