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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D98710: [clang-tid... Chih-Hung Hsieh via Phabricator via cfe-commits
    • [PATCH] D98710: [clan... Sam McCall via Phabricator via cfe-commits

Reply via email to