[RFC] Provide better experience for new clang-tidy users

Clang Frontend clang-tidy
1

Abstract

This proposal aims to provide new users of clang-tidy a smoother and better first-time experience with the tool.

Motivation

When new users come to clang-tidy some questions arise quite often:

  • What is clang-diagnostic/clang-analyzer warnings?
  • What is the default config for clang-tidy?
  • What checks should I use?

As a community, we could provide a better experience of first clang-tidy uses by eliminating ambiguity and providing guidance on check usage.

Proposal

  1. Remove checks (clang-analyzer-* and clang-diagnostic-*, but not clang-diagnostic-error which is a hard compiler error) from being implicit default checks, originally suggested in issues/146482 by carlosgalvezp:
    clang-analyzer-* are not special compared to other checks, so I see no reason why they should have special treatment. They are also heavier than most regular checks, so there's a hidden performance penalty by default.
    With this change, clang-tidy will have more predictable behavior regarding what checks it runs. For this change, we need to remove this code part

  2. Introduce a set of default checks for users to start with. This default set of checks should make a brief demonstration of what clang-tidy can do.
    These default checks could be presented to the user in 2 different ways:

    2.1) Activate default checks only when no .clang-tidy configuration file exists in the project and there is no explicit --checks argument provided.
    clang-tidy would display an informational message stating ā€œNo ā€˜.clang-tidyā€™ file or ā€˜ā€“checksā€™ were provided, using default checksā€ and proceed to run default checks.

    2.2) Make new option --dump-default-config or --create-default-config that will create a new .clang-tidy config file with default checks. clang-tidy will ask user to run this command if it didnā€™t find any .clang-tidy or --checks.

  3. Decide on the default checks.
    Iā€™d like to keep it small and avoid regex patterns:

bugprone-unused-return-value bugprone-empty-catch bugprone-redundant-branch-condition bugprone-unused-raii bugprone-use-after-move performance-move-const-arg readability-container-size-empty readability-duplicate-include readability-redundant-casting readability-string-compare readability-static-definition-in-anonymous-namespace

Whatā€™s Next?

We should come to a consensus on each of three presented topics:

  1. Remove current default checks or not.
  2. How should we introduce default checks: 2.1 (default checks) or 2.2 (create default config).
  3. Default check list. (Could be postponed till 1 and 2 are implemented)

History:

1 Like
2

My opinion:

  1. Yes, remove implicit checks
  2. Yes, letā€™s go with first option (default set of checks)
  3. Default checks in post.
3

I think compiler errors (clang-diagnotisc) would be good to keep enabled as default. Sometimes we enable compiler warnings via Clang-Tidy to be able to suppress with NOLINT.

But I understand the use case of ā€œI donā€™t want to spend time making my code compile with Clangā€, even though I think compiler warnings are the bare minimum before even working on Clang-Tidy checks.

1 Like
4

clang-diagnostic-* checks could be a part of the default checks that will only run when there is no .clang-tidy or --checks passed to clang-tidy. Diagnostic are still less recognizable and ungoogleable to some extent.

I want to remove them from current default checks that are always added in every clang-tidy invocation:

# in llvm-project clang-tidy --dump-config --- Checks: "clang-diagnostic-*,clang-analyzer-*,-*, clang-diagnostic-*, llvm-*,

User can enable them explicitly, like in LLVM config.

1 Like
5
  1. As a user I wasnā€™t aware that you could disable the clang diagnostic checks. Iā€™m against removing them as they indicate setup problems that result in the code not being parsed correctly. I donā€™t consider this checks.

Iā€™ve recently found a huge performance problem due to clang-static-analyzer. It also has an issue where it doesnā€™t show up in the performance profile. (I know itā€™s logged on GitHub) This makes it even more unwanted to be enabled by default. I completely agree with removing that default.

  1. There already exists an option to dump the config, so when no .clang-tidy file or command line option is given, I would expect that to dump the default config, Iā€™m not sure why you need another option, unless you really want to ignore those customizations.

What would be handy is dumping the documentation of the checks (names might be self-explanatory that this doesnt add value) and the options, something like:

Checks: - bugprone-assert-side-effect # Finds assert() with side effect. The condition of assert() is evaluated only in debug builds so a condition with side effect can cause different behavior in debug / release builds. CheckOptions: - bugprone-assert-side-effect.AssertMacros: "assert,NSAssert,NSCAssert" # A comma-separated list of the names of assert macros to be checked.

That way users donā€™t have to open the website to check the details of the options.

I would also suggest to dump all global options instead op the same option that can be overridden on several checks. Something like header file extensions doesnā€™t make much sense to be repeated and overwhelms the user trying to update the config file.

Aliases are another thing surprising for users. Checks are executed twice, while reporting of the results suggests that this is only once. It also comes with double the config options.

Iā€™m OK with requiring the user to provide checks, if it is easy to create a config file. I do worry that users would use --checks=* when they donā€™t want a config file.

For sure, ignoring the defaults when options are provided makes sense. Itā€™s really odd to write --checks=-*,bugprone-* to get the expected behavior.

In summary, next to some extra improvements, I am in favor of 2.1 combined with 2.2

  1. I find the concept of defaults really difficult as it requires us guessing what the user wants. Iā€™d be more inclined to say bugprone-* as people looking into static analysis are most likely looking for a tool that finds bugs, performance/readability/ā€¦ are all added values.

Selecting a few checks that donā€™t contain regex fields like cheating as the performance hit will come once they will actually use it.

6

I propose only removing default clang-diagnostic-* checks that are ā€œcompiler flagā€ checks. E.g. clang-diagnostic-literal-conversion is a ā€œcheckā€ enabled via -Wliteral-conversion. clang-tidy will still emit clang-diagnotic-error when unable to parse code. Iā€™ll adjust the description.

UPD: you canā€™t disable clang-diagnostic-error even with -clang-diagnostic-*.

2 Likes
7

The only change in 1. would be to remove these lines:

clang-diagnostic-* will still be available in config, like this:

8

The main difference between these two options is that when running clang-tidy with no .clang-clang or --checks, 2.1 will go implicitly with default checks, but 2.2. will show error message ā€œno checks providedā€ and suggest bootstrapping ā€œdefaultā€ .clang-tidy config. In 2.2, current --dump-config option will show empty config, but 2.1. will show default checks.

So 2.1 seem to be easier in general, but I preserved 2.2 as It was my original idea and maybe somebody would like it more.

As for current discourse and GitHub discussions, 2.1 seems a 100% winner.

1 Like
9

There were RFCs trying to address this problem and I intentionally kept it out of scope.
Aliases is bottomless trench.

1 Like
10

My initial concern with enabling all buprone-* checks is some of them may be too noisy and ā€œscaryā€ for unprepared codebases. To name a few:
bugprone-implicit-widening-of-multiplication-result - could be too strict for unprepared codebase.
bugprone-easily-swappable-parameters - many warning on perfectly fine code.

On the other hand, It could be a good indicator to go and create custom config.
So Iā€™m 50/50 convinced with bugprone-*.

1 Like
11

The reason I said both is because in the second one, you also want the defaults to be removed.

12

The only change in 1. would be to remove these lines:

Iā€™m AFK so I canā€™t verify this, but I believe itā€™s not trivial to disable the clang-diagnostic ones, at least the clang-diagnostic-error ones (which often come up if one has -Werror in compiler warnings). This is because these diagnostics come from another part of the code, not clang-tidy itself. But I could be wrong.

Edit: just saw a comment above confirming these canā€™t be disabled via regular -clang-diagnostic-error (which is a bit what the source code mentioned my @vbvictor above does).

13

Given this, Iā€™m OK with removing the clang-diagnostic-*. The errors are the thing I want to preserve. The warnings are less relevant to me. Just wondering, did you consider renaming these to clang-compiler-warnings-*? I suspect thatā€™s going to be much easier to understand. (Keeping the error name as is)

14

Good idea on the renaming, thank you. But it should be out of scope of this RFC.

Rename clang-diagnostic-error to clang-compiler-error can be done fairly easy: nobody can interact with it in .clang-tidy config so it will be an easy visual change.

For ā€œwarningsā€ clang-diagnostic-* we will need to provide a smooth transition because many projects already depend on them.

2 Likes
15

As per Hyrumā€™s law, there will be people relying on this name :slight_smile: For example, we post process the clang-tidy log and look for that string among other things.

But maybe it does not need as careful a transition as e.g. removing the clang-analyzer- checks.

1 Like