[RFC][clang-tidy] Additional checks to enable clang diagnostics

Clang Frontend clang-tidy
1

The current interface

Right now to enable additional clang diagnostics, one has to specify extra arguments on either the command line, or in the .clang-tidy config file.

This means invocations like the ones below:

  • clang-tidy --checks="bugprone-*" foo.c -- -Wall
  • clang-tidy --checks="bugprone-*" --extra-arg="-Wall" foo.c --

As well as config files like the one below.

# .clang-tidy Checks: bugprone-* ExtraArgs: ["-Wall"]

The proposed checks

The idea is to merge this functionality with the commonly used checks interface, so the invocation for enabling the -Wall flag would look like this:

clang-tidy --checks="clang-additional-diagnostic-all" foo.c --

This would allow users to control additional diagnostics and checks with the same interface. Also with this change it would be possible to enable multiple common warnings with only one check instead of having to type out each of them separately.

- clang-tidy --extra-arg="-Wunused-but-set-parameter" --extra-arg="-Wunused-command-line-argument" ... + clang-tidy --checks="clang-additional-diagnostic-unused-*" ...

Similarity with existing checks

The clang-additional-diagnostic name was chosen to avoid interfering with the already existing clang-diagnostic checks, which can be used to filter default warning messages.

clang-tidy --checks="-*,clang-diagnostic-return-type,..." foo.c --

The above call means that none of the default warnings reported by clang are shown to the user except for -Wreturn-type.

Check to flag mapping

The clang-additional-diagnostic checks would map like this:

  • --checks="clang-additional-diagnostic-unused" → -Wunused
  • --checks="-clang-additional-diagnostic-unused" → -Wno-unused

Optionally we could allow passing values to the warning flags as well, although I think only one warning allows it at the moment:
--checks="clang-additional-frame-larger-than" --config="{CheckOptions: [{key: clang-additional-frame-larger-than, value:X}]}" → -Wframe-larger-than=X

RFC

The idea came up during a discussion in a PR, and I wonder what the community thinks about implementing such a change.

Would you find it helpful?

1 Like
2

So basically idea is replace -W with clang-additional-diagnostic- for all clang diag warning?

What about the priority:

  • checks in CLI
  • checks in config
  • compilation database
  • extra arg in CLI
  • extra arg in config
3

It wouldn’t replace the current way of doing it, so adding -W to the compilation database, or passing it with --extra-arg would still be accepted, it is just an additional convenience feature.

One of the reasons a new check is introduced and it’s not the clang-diagnostic check modified is to ensure that every clang-tidy configuration that currently works will keep working as it is.

Right now it might be confusing that default warnings are filtered using the clang-diagnostic check, but any additional warnings require a different interface to be enabled.

4

these new checks support enable and disable (by -…). We need to consider the expected behavior when the check, flag in database, extra arg have some conflicts. e.g. should clang-tidy warn when a flag enabled by checks but disabled by compilation database.

For me, since these are not orthogonal, it is difficult to define an intuitive priority.

5

Another thing to consider is that today, we enable clang-diagnostic-* warnings. If we were to implement this feature, this would be equivalent to people having -Weverything in their code, which is likely not what they want.

I agree with the sentiment that this introduces extra complexity with regards to priority.

Also, it’s unclear how people would know what checks exist, we certainly don’t want to add them to the list of existing clang-tidy checks. They would need to jump back and forth between the clang-tidy and clang diagnostics documentation.

IMO I don’t see the problem in having clear boundaries for each tool: some checks are compiler warnings, some other checks are clang-tidy checks. Compiler warnings are enabled via -W*, which can be added via ExtraArgs, and clang-tidy checks are enabled via the existing API.

2 Likes
6

This has a well-defined behavior currently.

 --extra-arg=<string> - Additional argument to append to the compiler command line --extra-arg-before=<string> - Additional argument to prepend to the compiler command line

--extra-arg appends to the command line, so it overrides everything parsed from the compilation database, while --extra-arg-before prepends, so everything set by it is overridden by the compilation database.

For the current clang-diagnostic check, it’s also a well-defined behavior, e.g.: --checks="-clang-diagnostic-return-type" will hide the warnings from -Wreturn-type, regardless of what --extra-arg or the compilation database says. I think we should keep this behavior.

The check would be called clang-additional-diagnostic, so no collision here. Your mentioned use-case was the reason for giving the check a different name.

This applies to the existing clang-diagnostic check as well. We can just add another line next to the documentation of that check on this page. IIUC this is the only place that check is documented.

My concern is that if we want to enable multiple checks together, but want to exclude one of them, we need to use 2 different interfaces.

Imagine we want to enable -Wextra, but still exclude for example -Wunused-parameter for some reason. Right now it might look like this:

clang-tidy --checks="-clang-diagnostic-unused-parameter, ..." --extra-arg=-Wextra main.c --

In a configuration file it might look like this:

# .clang-tidy Checks: -clang-diagnostic-unused-parameter, ... CheckOptions: ... ExtraArgs: ["-Wextra"]

In this case -clang-diagnostic-unused-parameter and -Wextra logically tie together, yet this is not implied.

With the new checks the command line case would look like this:

clang-tidy --checks="clang-additional-diagnostic-extra,-clang-additional-diagnostic-unused-parameter, ..." main.c --

While the config file would look like this:

# .clang-tidy Checks: clang-additional-diagnostic-extra, -clang-additional-diagnostic-unused-parameter, ... CheckOptions: ...

It might be easier for the eye to understand that these checks are related.

7

Then we do what we always do with compiler warnings, no need for an extra interface?

ExtraArgs = [ '-Wextra', '-Wno-unused-parameter', ]
2 Likes
8

True. Let’s leave everything as it is.

9

extra arg is clear because we have clear order of command line arguments. But for the clang-diagnostics.

What is the expected behavior when code has unused parameter

  1. database + extra-arg has -Wunused-parameter, checks is -clang-diagnostic-unused-parameter
  2. database + extra-arg has -Wno-unused-parameter, checks is clang-diagnostic-unused-parameter
  3. database + extra-arg has -Wunused-parameter, checks is -clang-diagnostic-extra
  4. database + extra-arg has -Wno-unused-parameter, checks is clang-diagnostic-extra
  5. database + extra-arg has -Wextra, checks is -clang-diagnostic-unused-parameter