[PATCH] D98710: [clang-tidy] New feature --skip-headers, part 1

Tue Jul 13 06:23:06 PDT 2021

sammccall added a comment.

In D98710#2873141 <https://reviews.llvm.org/D98710#2873141>, @chh wrote:

> Sam, the revision summary is updated. Could you review it again?

Just to clarify - the code hasn't changed since last review pass right? I think the comments there still apply. Happy to review the patch description though!

> This new feature has impacts similar to --header-filter and --system-headers.

I don't think it does a similar thing to those flags, rather it mostly affects how those flags work. (The stuff about diagnostic counts is details that aren't really the "top-line" of this feature).
I'd say rather something like:

> The `--skip-headers` mode changes how the flags `--header-filter` and `--system-headers` limit the scope of checks.`
> With `--skip-headers=0` (old behaviour; default), checkers inspect all code, but warnings in files out of scope are not reported.
> With `--skip-headers=1`, checkers do not inspect code from files that are out of scope. This can be a significant performance improvement.

> Add a --show-all-warnings flag. It is hidden, not shown by --help. When it is used, ClangTidyDiagnosticConsumer::checkFilters will not suppress any diagnostic message. This is useful to find tidy checks that have not yet handled the --skip-headers flag.

This doesn't look right - an important part of the design is that tidy checks shouldn't need to be modified. (Though it's possible to imagine checks that wouldn't work properly in this mode)
Maybe rather "useful in conjunction with --skip-headers to find checks that may be processing code that should rather be skipped"?

> If there is such a case in the future, we might need some other method to skip checks without cutting out Decls in AST

Realistically, I think we should probably just say such cases are unsupported. We believe no such checks exist in-tree, and clang-tidy upstream can't really be constrained by what people are doing out-of-tree.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98710/new/

https://reviews.llvm.org/D98710