clang-tidy does not complain about missing commas in config file

[tpudlik]

The following .clang-tidy file will enable certain checks:

---
Checks: >
    bugprone-argument-comment,
    bugprone-assert-side-effect,
    bugprone-bool-pointer-implicit-conversion,
...

The following .clang-tidy file (missing the trailing commas) will enable no checks, but clang-tidy will not complain:

---
Checks: >
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion
...

It is easy to forget to add a trailing comma in a long list, accidentally disabling checks (real-world example). Clang-tidy should perform some validation of the config to catch such bugs.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[llvmbot]

@llvm/issue-subscribers-bug

[SimplyDanny]

I've created a fix allowing newline characters as separators of checks.

[SimplyDanny]

clang-tidy now considers checks as being distinct if they are either separated by a comma or by a newline character. That means that

Checks: >
    bugprone-argument-comment,
    bugprone-assert-side-effect,
    bugprone-bool-pointer-implicit-conversion

means the same as

Checks: |
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion

or Checks: bugprone-argument-comment,bugprone-assert-side-effect,bugprone-bool-pointer-implicit-conversion.

clang-tidy will not warn about missing commas. Instead, it will accept such lists more naturally even without commas as separators. Is this an acceptable solution for you, @tpudlik?

[tpudlik]

Thank you! Just to clarify, what will happen if someone uses the folded style indicator (>), but forgets some of the commas? Example:

Checks: >
    bugprone-argument-comment,
    bugprone-assert-side-effect # Note, no comma!
    bugprone-bool-pointer-implicit-conversion,
    yet-another-check

The issue that originally prompted this bug was of this form (some commas were missing in a long list of checks).

Looking at the test in https://reviews.llvm.org/D120187, it appears this config will now be parsed correctly, but I wanted to double-check.

[SimplyDanny]

This specific example will only be correctly parsed if the pipe character | is used. The point here is that the YAML parser does its thing and returns simple strings where the > and | are already resolved. Only then the checks are split at newlines (if | was used) or commas or both.

I guess, the general advice now is to go with | instead of > to avoid such errors.

[bilderbuchi]

clang-tidy now considers checks as being distinct if they are either separated by a comma or by a newline character. That means that

Checks: >
    bugprone-argument-comment,
    bugprone-assert-side-effect,
    bugprone-bool-pointer-implicit-conversion

means the same as

Checks: |
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion

or Checks: bugprone-argument-comment,bugprone-assert-side-effect,bugprone-bool-pointer-implicit-conversion.

clang-tidy will not warn about missing commas. Instead, it will accept such lists more naturally even without commas as separators.

@SimplyDanny has this fix made it into the codebase correctly?

I'm on 15.0.7, and with a .clang-tidy containing

---
Checks: |
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion
...

gives

clang-tidy --verify-config
/somepath/.clang-tidy: warning: unknown check 'bugprone-argument-comment
bugprone-assert-side-effect
bugprone-bool-pointer-implicit-conversion' [-verify-config]

while

---
Checks: >
    bugprone-argument-comment,
    bugprone-assert-side-effect,
    bugprone-bool-pointer-implicit-conversion
...

gives

clang-tidy --verify-config
No config errors detected.

 clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 15.0.7
  Optimized build.
  Default target: x86_64-alpine-linux-musl
  Host CPU: tigerlake

[SimplyDanny]

The fix/feature made it into the release back than, but as I've just checked it doesn't work anymore.

[bilderbuchi]

The test for this is still in the codebase. I could not figure out if it is actually still run -- I'm unfamiliar with the LLVM infra, so can't help with that, sorry.