[clang-tidy] Dealing with check names

Hi,
as clang-tidy grows there are more and more checks. One of the problem I see is that “misc” check group is not user friendly - there are many checks that do so many different things that usually user don’t want to enable whole group.
Other groups like modernize, performance, google, cert, boost, llvm doesn’t have this problem. Naturally the solution would be to split the group into smaller groups that would mean more.
The problem is that we should not change names because old configs will not work.

Do you have some ideas how we could fix it, so we could make it easier for users to use it?

Other feature that we could add if we would know how to solve it is that we could make new groups that would mostly have aliases to other checks. This might be specially useful for cert checks - the cert code names doesn’t tell anything, so it would be good to have these checks with proper name in different group so normal user could see what this check is doing from name and CERT users could run checks with cert group as it was before.

One solution that I see is to reserve old name and make redirection, and maybe output warning about deprecated name when user would use special flag (e.g. verbose)

What do you think about this problem?

Piotr

Hi!

What about an alternative solution, like changing the names and providing a python script to migrate the configuration files?

Regards,

Gábor

This sounds also good. I was thinking that it would be nice if clang-tidy itself could warn about old name and dump config, but problem here is
when user have configuration “misc-*” and we moved one check from misc to other group. We should probably dump clang-tidy version in the config to know how old is config.

That is a hairy problem. The reason why I do not like too many aliases, because it makes it harder to introduce clang tidy to a project. One have to go through more checkers. Also more work to set up the configuration files and check for inconsistencies (e.g.: same checker turned on using multiple names and inconsistent configuration options).

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAEhrgAPqzCPZhtJ7qbgk+V-Vc_T-wSBsgPWHg74dj4QSrg4SZQ@mail.gmail.com>,
    Piotr Padlewski via cfe-dev <cfe-dev@lists.llvm.org> writes:

This sounds also good. I was thinking that it would be nice if clang-tidy
itself could warn about old name and dump config, but problem here is
when user have configuration "misc-*" and we moved one check from misc to
other group. We should probably dump clang-tidy version in the config to
know how old is config.

I believe clang-tidy already has a mechanism for saying "this check is
an alias of this other check", so you can migrate the checks to a new
organization and keep the old names as aliases for the new names.

I don't know if it emits any kind of diagnostic that you are using an
old alias. We could introduce a way of deprecating a check name and
emit if you use the deprecated name.

Cześć, Piotr!

Hi,
as clang-tidy grows there are more and more checks. One of the problem I see
is that "misc" check group is not user friendly - there are many checks that
do so many different things that usually user don't want to enable whole
group.
Other groups like modernize, performance, google, cert, boost, llvm doesn't
have this problem. Naturally the solution would be to split the group into
smaller groups that would mean more.
The problem is that we should not change names because old configs will not
work.

Do you have some ideas how we could fix it, so we could make it easier for
users to use it?

Other feature that we could add if we would know how to solve it is that we
could make new groups that would mostly have aliases to other checks. This
might be specially useful for cert checks - the cert code names doesn't tell
anything, so it would be good to have these checks with proper name in
different group so normal user could see what this check is doing from name
and CERT users could run checks with cert group as it was before.

One solution that I see is to reserve old name and make redirection, and
maybe output warning about deprecated name when user would use special flag
(e.g. verbose)

What do you think about this problem?

Piotr

It's really good idea to split misc checks!

My suggestions:

I think it’ll be good idea to move unused checks to Clang, but will
this be easy? Clang has warnings for unused variables, static
functions and macros already. GCC has warning for unused parameters.

misc-unused-alias-decls
misc-unused-parameters
misc-unused-raii
misc-unused-using-decls

misc-inefficient-algorithm should be moved to performance. I tried to
do this in past, but my patch was not perfect.

Probable we should create next check groups:

Potential memory issues:

misc-use-after-move

Preprocessor:

misc-macro-parentheses
misc-macro-repeated-side-effects
misc-multiple-statement-macro

Casts:

misc-bool-pointer-implicit-conversion
misc-incorrect-roundings
misc-misplaced-widening-cast

Eugene.

Cześć, Piotr!

> Hi,
> as clang-tidy grows there are more and more checks. One of the problem I
see
> is that "misc" check group is not user friendly - there are many checks
that
> do so many different things that usually user don't want to enable whole
> group.
> Other groups like modernize, performance, google, cert, boost, llvm
doesn't
> have this problem. Naturally the solution would be to split the group
into
> smaller groups that would mean more.
> The problem is that we should not change names because old configs will
not
> work.
>
> Do you have some ideas how we could fix it, so we could make it easier
for
> users to use it?
>
> Other feature that we could add if we would know how to solve it is that
we
> could make new groups that would mostly have aliases to other checks.
This
> might be specially useful for cert checks - the cert code names doesn't
tell
> anything, so it would be good to have these checks with proper name in
> different group so normal user could see what this check is doing from
name
> and CERT users could run checks with cert group as it was before.
>
>
> One solution that I see is to reserve old name and make redirection, and
> maybe output warning about deprecated name when user would use special
flag
> (e.g. verbose)
>
> What do you think about this problem?
>
> Piotr

It's really good idea to split misc checks!

My suggestions:

I think it’ll be good idea to move unused checks to Clang, but will
this be easy? Clang has warnings for unused variables, static
functions and macros already. GCC has warning for unused parameters.

misc-unused-alias-decls
misc-unused-parameters
misc-unused-raii
misc-unused-using-decls

Or maybe it should be in one check with options? Maybe only

misc-unused-parameters is different enough to have special option to
disable it.

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAEhrgAPqzCPZhtJ7qbgk+V-Vc_T-wSBsgPWHg74dj4QSrg4SZQ@mail.
gmail.com>,
    Piotr Padlewski via cfe-dev <cfe-dev@lists.llvm.org> writes:

> This sounds also good. I was thinking that it would be nice if clang-tidy
> itself could warn about old name and dump config, but problem here is
> when user have configuration "misc-*" and we moved one check from misc to
> other group. We should probably dump clang-tidy version in the config to
> know how old is config.

I believe clang-tidy already has a mechanism for saying "this check is
an alias of this other check", so you can migrate the checks to a new
organization and keep the old names as aliases for the new names.

I know about the aliases.

I don't know if it emits any kind of diagnostic that you are using an
old alias. We could introduce a way of deprecating a check name and
emit if you use the deprecated name.

The problem that I mentioned, if we move one check and someone is enabling
it with asterisk ("misc-*")
then having the todays config we would not know if the user meant to use
old check or not.

users don't want to enable whole google/llvm/cert/cppcoreguidelines group.
Instead they want to pick "performance" or "readability" or "modernize".

Example:

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAEhrgAML38UG77j3kUAKYFSmH0RkcyPq06cXUrkj1tZeG+Z-jQ@mail.gmail.com>,
    Piotr Padlewski via cfe-dev <cfe-dev@lists.llvm.org> writes:

   - These checks could be moved into readability
   llvm-header-guard
   <http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html&gt;
   - llvm-include-order
   <http://clang.llvm.org/extra/clang-tidy/checks/llvm-include-order.html&gt;
   - llvm-namespace-comment

No, they can't, because they are enforcing LLVM's style guidelines.

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAEhrgAML38UG77j3kUAKYFSmH0RkcyPq06cXUrkj1tZeG+Z-jQ@mail.
gmail.com>,
    Piotr Padlewski via cfe-dev <cfe-dev@lists.llvm.org> writes:

> - These checks could be moved into readability
> llvm-header-guard
> <http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html
>
> - llvm-include-order
> <http://clang.llvm.org/extra/clang-tidy/checks/llvm-
include-order.html>
> - llvm-namespace-comment

No, they can't, because they are enforcing LLVM's style guidelines.

Is LLVM style guide so special that no one can use it outside of LLVM? LLVM
specific checks would still be visible and would do the same thing, but
they would be links to more generic checks that people outside could use.
Or maybe you prefer to put iit-namespace-comment,
myawesomecompany-namespace-comment etc. LLVM is not the only one project
that uses namespace comments.

We already have google-readability-namespace-comments and
llvm-namespace-comment checks, that configure the same check in
different ways.

I don't know why Google don't use .clang-tidy to configure generic
checks how they want them.

> Is LLVM style guide so special that no one can use it outside of LLVM?
LLVM
> specific checks would still be visible and would do the same thing, but
they
> would be links to more generic checks that people outside could use.
> Or maybe you prefer to put iit-namespace-comment,
> myawesomecompany-namespace-comment etc. LLVM is not the only one project
> that uses namespace comments.

We already have google-readability-namespace-comments and
llvm-namespace-comment checks, that configure the same check in
different ways.

I don't know why Google don't use .clang-tidy to configure generic
checks how they want them.

I don't mind having google/llvm specific checks in list (as links),

because google is the biggest contributor to clang-tidy and parts of google
coding standard are uses outside of google too.

Alex, do you have a comment about the topic? I really would like to know if there is something I can do about grouping checks into new groups and changing names of checks.

Piotr

Hi,
as clang-tidy grows there are more and more checks. One of the problem I see
is that "misc" check group is not user friendly - there are many checks that
do so many different things that usually user don't want to enable whole
group.
Other groups like modernize, performance, google, cert, boost, llvm doesn't
have this problem. Naturally the solution would be to split the group into
smaller groups that would mean more.
The problem is that we should not change names because old configs will not
work.

Do you have some ideas how we could fix it, so we could make it easier for
users to use it?

I think that trying to reduce the craziness in the misc module is a
good goal, thank you for bringing this up!

The way we've solved this in the frontend is to not try to make the
groupings overly large, but instead base the groupings on something
tangible. For instance, we have -Wunused to enable a slew of unused
diagnostics of varying kinds, -Wdeprecated for various deprecated
diagnostics, etc. However, we're also fine having diagnostic "groups"
that contain only a single member because not all diagnostics are
easily categorized. Perhaps we could use a similar mechanism for
clang-tidy, instead of trying to come up with only large groupings?

Some checks are going to be very easy to group together, like coding
standards (cert, cppcoreguidelines, google, llvm). Other checks are
still pretty easy to group together because they relate to a shared
goal (readability, performance). But I think that a lot of checks are
essentially unrelated, and trying to force them into a categorization
will always be a losing battle.

Other feature that we could add if we would know how to solve it is that we
could make new groups that would mostly have aliases to other checks. This
might be specially useful for cert checks - the cert code names doesn't tell
anything, so it would be good to have these checks with proper name in
different group so normal user could see what this check is doing from name
and CERT users could run checks with cert group as it was before.

We already have this functionality, so I'm not certain what you are
proposing to add. Also, the CERT names do tell you information -- they
map to specific CERT coding guideline names, which have a unique,
stable identifier. They mean something to people trying to conform to
CERT's coding guidelines.

One solution that I see is to reserve old name and make redirection, and
maybe output warning about deprecated name when user would use special flag
(e.g. verbose)

What do you think about this problem?

I think that aliases to other checks are orthogonal to the problem of
grouping checks together. Certainly, I think that aliases are useful;
we already have checks that are shared between modules, such as checks
used by coding standards. However, I don't think that we will ever
have a satisfactory large-scale grouping for all of our checks without
a "misc" category, and that category is almost always going to be the
default for new checks (which is what we see in practice in the
frontend). I think we should not attempt to reinvent the wheel and
instead use the grouping concepts from the frontend, which would
likely do away with the misc- category entirely.

~Aaron

Hi,
as clang-tidy grows there are more and more checks. One of the problem I
see is that "misc" check group is not user friendly - there are many checks
that do so many different things that usually user don't want to enable
whole group.

Other groups like modernize, performance, google, cert, boost, llvm doesn't

have this problem. Naturally the solution would be to split the group into
smaller groups that would mean more.

Most checks in "misc" could be better described as "checks targeting
bug-prone patterns" and moved to a new module (e.g. "bugprone"). That would
make things better.

The problem is that we should not change names because old configs will
not work.

Following this logic, we should not add new checks or add/remove/change
check options, since some configs could stop working as they used to. Is
there an evidence of this being a serious issue?

Do you have some ideas how we could fix it, so we could make it easier for
users to use it?

Other feature that we could add if we would know how to solve it is that
we could make new groups that would mostly have aliases to other checks.
This might be specially useful for cert checks - the cert code names
doesn't tell anything, so it would be good to have these checks with proper
name in different group so normal user could see what this check is doing
from name and CERT users could run checks with cert group as it was before.

One solution that I see is to reserve old name and make redirection, and
maybe output warning about deprecated name when user would use special flag
(e.g. verbose)

I personally don't think adding a deprecation mechanism for clang-tidy
checks is valuable. We could instead implement some form of validation of
check patterns (which could, for example, try to remove each part of the
glob list and see whether the resulting set of enabled checks changes).

I like the way clang-format handles this; the --style option allows different organisations to have different default settings.

How about removing the aliases and having --defaults={llvm,google,cert,cppcoreguidelines} select a default set of checks and options.

Hi,
as clang-tidy grows there are more and more checks. One of the problem I
see is that "misc" check group is not user friendly - there are many checks
that do so many different things that usually user don't want to enable
whole group.

Other groups like modernize, performance, google, cert, boost, llvm

doesn't have this problem. Naturally the solution would be to split the
group into smaller groups that would mean more.

Most checks in "misc" could be better described as "checks targeting
bug-prone patterns" and moved to a new module (e.g. "bugprone"). That would
make things better.

On the other hand, more than half checks could be categorized to that. Even
some readability and modernize checks could be categorized this way because
they find code code that is bugprone and change it to something that is
less bugprone (either more modern or more readable). I think we need to go
deeper and find some more specific names.

The problem is that we should not change names because old configs will
not work.

Following this logic, we should not add new checks or add/remove/change
check options, since some configs could stop working as they used to. Is
there an evidence of this being a serious issue?

Good point.

o categorization of check if we would like to make this tool very easy to
use.
I would like to have some kind of flag that would use some default checks,
that we know are useful for < 95% of users, have very little false
positives.
Each level (let say 4 levels like with optimizations) would have different
set of checks, where -W0 would be as good as -Wall in frontend (it would be
extended to checks that are useful but can't be in clang because analysis
would take too long). One check could be enabled in multiple levels with
different options, f.e.
modernize-loop-convert in -W0 would use MinConfidence: safe, and in -W1:
resonable and in -W3: risky.

I know that now we would have to get to consensus about which checks are
usefull on which level, but because the levels would be something that user
should not relay on, we could change the configuration every time someone
fix a bug or report a bug. And the same thing is in frontend (if warning
should go to -Wall or not) and people get to consensus what diagnostic is
good enough to be in which group.

Piotr