Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying the Checks as a list in the yaml config file #51428

Closed
llvmbot opened this issue Oct 5, 2021 · 5 comments
Closed

Allow specifying the Checks as a list in the yaml config file #51428

llvmbot opened this issue Oct 5, 2021 · 5 comments
Labels
bugzilla Issues migrated from bugzilla clang-tidy

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 5, 2021

Bugzilla Link 52086
Version unspecified
OS Linux
Reporter LLVM Bugzilla Contributor
CC @whisperity

Extended Description

Let's say I want to say:

  • Enable all the hicpp-* and cppcoreguidelines-* checks
  • With the exception of cppcoreguidelines-narrowing-conversions, because I don't like it
  • And without hicpp-avoid-goto because it's just a duplicate of cppcoreguidelines-avoid-goto and I don't want to have to add both in my NOLINT comments.

Currently I can do


Checks: 'hicpp-,cppcoreguidelines-,-cppcoreguidelines-narrowing-conversions,-hicpp-avoid-goto'
...

but it's not clear cppcoreguidelines-narrowing-conversions and hicpp-avoid-goto have been disable for very different reasons. I could add a comment before "Checks", but that doesn't work too well when the list gets longer.

As far as I know there is no way to include comments interleaving a YAML string. So it would be nice if "Checks" could be expressed also as a list, so


Checks:

  • 'hicpp-,cppcoreguidelines-'

I don't like it

  • '-cppcoreguidelines-narrowing-conversions'

Duplicate of cppcoreguidelines-avoid-goto

  • '-hicpp-avoid-goto'
    ...

could be used.

@whisperity
Copy link
Member

(Side note: of course this highlights another issue, namely that NOLINT should also check for aliases when deciding whether to silence a diagnostic?)

@llvmbot
Copy link
Collaborator Author

llvmbot commented Oct 6, 2021

That would indeed be nice.

The aliases also have the problem that it's not too clear from the docs how they work. I don't think the following question has an answer in the docs:

Is

clang-tidy --checks='-*,hicpp-avoid-goto,cppcoreguidelines-avoid-goto' test.cpp

going to take double the time than

clang-tidy --checks='-*,cppcoreguidelines-avoid-goto' test.cpp

to do the exact same work?

By measuring the wall time of both commands with a (too small to create reliable measurements) test file I would say the answer is no, that clang-tidy is smart enough to notice they are the same and only does the check once. But if I ask --enable-check-profile

clang-tidy --checks='-*,hicpp-avoid-goto,cppcoreguidelines-avoid-goto' --enable-check-profile test.cpp

I get things like

===-------------------------------------------------------------------------===
clang-tidy checks profiling
===-------------------------------------------------------------------------===
Total Execution Time: 0.0001 seconds (0.0001 wall clock)

---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
0.0001 ( 80.3%) 0.0000 ( 0.0%) 0.0001 ( 79.7%) 0.0001 ( 81.2%) hicpp-avoid-goto
0.0000 ( 19.7%) 0.0000 (100.0%) 0.0000 ( 20.3%) 0.0000 ( 18.8%) cppcoreguidelines-avoid-goto
0.0001 (100.0%) 0.0000 (100.0%) 0.0001 (100.0%) 0.0001 (100.0%) Total

Which seem to suggest the aliased checks slows down the process, at least a bit.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@dawagner
Copy link

I would also like this. At my work, we've grown a homemade wrapper of clang-tidy that parses a yaml file supporting Checks as a list as well as overlays. Overlays are obsolete now that InheritParentConfig has been introduced but we still can't get rid of the wrapper.

Note that the following would work:

Checks: >-
  *,
  -some-alias,

but we also want to add some comments in there, and that's not possible whereas it can be done if Checks was a yaml list.

@carlosgalvezp
Copy link
Contributor

It is certainly possible to have a list-like with interleaved comments:

Checks: >
  *,
  # Do not run this module,
  -some-module*,
  # Except this check,
  some-module-foo,

The trick is that the comment must end with a comma. Arguably not the most readable/maintainable though.

@carlosgalvezp
Copy link
Contributor

Patch up for review: https://reviews.llvm.org/D147876

@carlosgalvezp carlosgalvezp added the awaiting-review Has pending Phabricator review label Apr 9, 2023
@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label Apr 10, 2023
gysit pushed a commit to nextsilicon/llvm-project that referenced this issue Apr 27, 2023
Specifying checks as a string is convenient for quickly using
clang-tidy to run a handful of checks. However it is not
suitable for projects that have a long list of enabled or
disabled checks. It is specially troublesome in case one
wants to interleave comments with the checks, to explain
why they are enabled or disabled.

Currently this can be achieved via multiline strings in YAML,
but it's error-prone. For example, comments must end with a
comma for clang-tidy to continue processing the list of globs;
a missing comma will make clang-tidy silently ignore the rest
of the list.

Instead, enable passing a native YAML list to the "Checks"
option in the config file. The implementation is done such
that the old behavior is kept: a user can pass a string
or a list. We can consider deprecating passing the checks
as a string altogether in a future release, to simplify
the internal logic of the YAML parser.

Fixes llvm#51428

Differential Revision: https://reviews.llvm.org/D147876
flemairen6 pushed a commit to Xilinx/llvm-project that referenced this issue May 10, 2023
Specifying checks as a string is convenient for quickly using
clang-tidy to run a handful of checks. However it is not
suitable for projects that have a long list of enabled or
disabled checks. It is specially troublesome in case one
wants to interleave comments with the checks, to explain
why they are enabled or disabled.

Currently this can be achieved via multiline strings in YAML,
but it's error-prone. For example, comments must end with a
comma for clang-tidy to continue processing the list of globs;
a missing comma will make clang-tidy silently ignore the rest
of the list.

Instead, enable passing a native YAML list to the "Checks"
option in the config file. The implementation is done such
that the old behavior is kept: a user can pass a string
or a list. We can consider deprecating passing the checks
as a string altogether in a future release, to simplify
the internal logic of the YAML parser.

Fixes llvm#51428

Differential Revision: https://reviews.llvm.org/D147876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang-tidy
Projects
None yet
Development

No branches or pull requests

4 participants