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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 362704.
chh added a comment.

sync to the latest source; apply clang-format; add new skip-headers-4.cpp test


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/a.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/a.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/c.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/c1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/unused-using-decls.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-2.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-3.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-4.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' 

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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 362239.
chh edited the summary of this revision.
chh added a comment.

Add new test cases into misc-unused-using-decls.cpp.


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/a.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/a.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/c1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/unused-using-decls.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-2.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-3.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I 

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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 361871.
chh added a comment.

add skip-headers-3.cpp test to show that checking only Decl locations is not 
enough to skip some warnings in header files


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/a.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/a.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/c1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-2.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-3.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
 
 #include "header1.h"
 // CHECK-NOT: header1.h:{{.*}} warning
Index: 

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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh added a comment.

Some Android developers and legacy code care less about clang-tidy warnings.
Newer developers spend a lot of time to get lint-free new code.
So Android source tree has a lot of clang-tidy flags like
header-filter, checks, and warnings-as-errors to select checks
for different modules. People are even asking for new features like
making header-filter to accept a list of regexp like the checks flag.

If a new change to header-filter or skip-headers lose existing valid warnings,
maybe it will be perfectly acceptable to some but not to people who want to
make code lint-free. To them, these changes are regressions.
A clang-tidy check's owner would not like to lose capability for
some minor performance gain, either.

bugprone-forward-declaration-namespace is just one valid warning found
in a couple of days. Android tree is huge and has disabled many checks,
so we won't know the impact to all checks any time soon. 
If we have a mechanism to keep any special checks from the impact,
like the PPCallback-based checks, it will be much safer to release 
the first phase skip-headers and know that we can fix any bad impact quickly.

I am trying such a mechanism to support "see-all-file" MatchFinder-based checks.
Those checks will be very few, so skip-headers still saves a lot of runtime.
After more tests, I will upload it to D98709 .

Please note that I added a new test skip-headers-2.cpp.
It is extremely simplified, but reflects the need to check a top-level
Decl and ignore warnings in included nested Decls in other files.
If we implement skip-headers based on only top-level Decls,
we won't be able to support such a use case.
In D98709 , we check/skip not only top-level 
Decls.


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-tidy] New feature --skip-headers, part 1, setTraversalScope

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 360651.
chh edited the summary of this revision.
chh added a comment.

Add one more skip-headers-2.cpp test, which shows desired check at a top-level 
Decl but not at nested Decl. Checking locations of only top-level Decls will 
miss the opportunity to skip nested Decls.


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/a.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/a.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers-2.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
 
 #include "header1.h"
 // CHECK-NOT: header1.h:{{.*}} warning
Index: 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Please before sending any more patches, let's resolve the design questions here 
(or via mail).

It's clear that it's possible to write a clang-tidy check whose behavior on the 
main file cannot be preserved without traversing the whole AST.
Silly example: the main file should have a constant like `int 
TotalNumberOfVarDeclsInThisTranslationUnit = 61523;`, warn if it's missing or 
incorrect.

Possibilities for dealing with this include:

1. traversing the whole AST
2. traversing a partial AST, and accepting some divergence in behavior
3. allowing each check to customize the strategy for subsetting the AST

So far we've been discussing #2, and hoping the divergences are 
small/acceptable. Are you saying that unless the divergence is zero, this 
feature is no use to you? And at a high level, what's your idea to address this?

Compared to the gold standard of traversing the whole AST, we can go wrong 
either by adding warnings we don't want (false positives), or missing warnings 
we want (false negatives). False positives are really bad for clang-tidy and I 
think we can't allow these to be common in practice. However false negatives 
with certain checks (e.g. the forward-declaration-namespace check) seem like an 
acceptable functionality vs performance tradeoff that's more than offset by the 
ability to run clang-tidy in more contexts.


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-tidy] New feature --skip-headers, part 1, setTraversalScope

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 360339.
chh edited the summary of this revision.
chh added a comment.

Add bugprone-forward-declaration-namespace-header.cpp test to
show that MatchFinder-based checks can also depend on header Decls.


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/a.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-forward-declaration-namespace/b.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
 
 #include "header1.h"
 // CHECK-NOT: header1.h:{{.*}} warning
Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -1,14 +1,14 @@
-// RUN: clang-tidy 

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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh added a comment.

Sam,

The latest tested change contained one of your suggestions:

  using setTraversalScope for all consumers in the
  ClangTidyASTConsumer::HandleTranslationUnit

It is good to pass all existing tests,
although not a proof of correctness yet.

The failed libarcher.* tests have been there for days.
We can be sure that those are unrelated to this change.

We still have some different views of the purpose and requirements of
these new flags, skip-headers and show-all-warnings. That's why it is
important to describe them correctly in the summary.
We can try to include more use cases or applications now or later,
however, the following has been our requirement for a successful story
for the first Android deployment:

  --skip-headers should work correctly, even if not at optimal speed.
  
  Correctness means no more or fewer "displayed" warnings with or without
  this flag, although it could report fewer "suppressed" header file warnings.

For desired performance gain, experiment data showed that it is more than
enough to get savings from only MatchFinder-based checks.

We are less critical on "implementation" methods, code complexity, or 
efficiency.
Using set/getTraversalScope or not, cutting Decls in advance or on the fly,
cutting only top-level Decls or all Decls at any level, are all acceptable
alternatives if they produce the same "displayed" warnings as before.

Please also take a look of https://reviews.llvm.org/D98709,
which skips Decls at all levels on-the-fly.
That is a different implementation with 50% less changes in ClangTidy.cpp
than this one. The changes in other files are identical or tiny.
It is a little slower, but D98709  is smaller 
and simpler to maintain,
and it limits the impact to only MatchFinder, not static analyzer.

Now the bad news is that when tested against the whole Android source, 
I found failed cases, which are missing clang-tidy warning messages
by both implementations.

The missed warnings were bugprone-forward-declaration-namespace.
There could be other misses undetected.
According to bugprone/ForwardDeclarationNamespaceCheck.cpp,
it's a MatchFinder-based check that can report warning on
Decl in the main file, but the check needs Decls in an
included file. For this kind of checks, skipping their
matchers for Decls in header files is wrong.
This is similar to what we found before that some checks
need the root TranslationUnit node in AST, so we have to
change set/getTraversalScope to keep the root node.

Now we realized that we cannot simply skip or cut out Decls
of headers files for ALL MatchFinder-based checks. Some checks
or some of their matchers need to see ALL Decls in ALL source files.

I will try to update this D98710  and D98709 
 with new test cases
to show the failed bugprone-forward-declaration-namespace checks.
Then, I will try some implementation changes to work also for
those tidy checks.  The implementation probably won't be as simple
as this one to cut all top-level header file Decls for all checks.


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-tidy] New feature --skip-headers, part 1, setTraversalScope

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 359532.
chh retitled this revision from "[clang-tidy] New feature --skip-headers, part 
1, (tested as default)" to "[clang-tidy] New feature --skip-headers, part 1, 
setTraversalScope".
chh edited the summary of this revision.
chh added a comment.

--skip-headers is set to false by default;
many tidy tests runs are augmented with explicit --skip-headers or 
--skip-headers=0
to test this feature as on or off.


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
 
 #include "header1.h"
 // CHECK-NOT: header1.h:{{.*}} warning
Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -1,14 +1,14 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' %s -- -I 

[PATCH] D98710: [clang-tidy] New feature --skip-headers, part 1, (tested as default)

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh updated this revision to Diff 359506.
chh retitled this revision from "[clang-tidy] New feature --skip-headers, part 
1" to "[clang-tidy] New feature --skip-headers, part 1, (tested as default)".
chh edited the summary of this revision.
chh added a comment.

to test skip-headers as default, do not submit;
show that setTraversalScope failed some static analyzer checks,
see #if 0 comments in ClangTidy.cpp.


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

https://reviews.llvm.org/D98710

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  
clang-tools-extra/test/clang-tidy/checkers/abseil-upgrade-duration-conversions.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m
  clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp
  clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  clang/include/clang/Frontend/MultiplexConsumer.h

Index: clang/include/clang/Frontend/MultiplexConsumer.h
===
--- clang/include/clang/Frontend/MultiplexConsumer.h
+++ clang/include/clang/Frontend/MultiplexConsumer.h
@@ -77,8 +77,9 @@
   void InitializeSema(Sema ) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector> Consumers; // Owns these.
+private:
   std::unique_ptr MutationListener;
   std::unique_ptr DeserializationListener;
 };
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
+// RUN: clang-tidy --skip-headers=0 -checks='-*,google-explicit-constructor' -line-filter='[{"name":"line-filter.cpp","lines":[[18,18],[22,22]]},{"name":"header1.h","lines":[[1,2]]},{"name":"header2.h"},{"name":"header3.h"}]' -header-filter='header[12]\.h$' %s -- -I %S/Inputs/line-filter 2>&1 | FileCheck %s
 
 #include "header1.h"
 // CHECK-NOT: header1.h:{{.*}} warning
Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -1,14 +1,14 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' %s -- -I %S/Inputs/file-filter -isystem 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In 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-tidy] New feature --skip-headers, part 1

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh added a comment.

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

The new summary would clarify a few points:

- This --skip-headers is a new feature, not just a transparent run-time saving. 
It should be used carefully with header-filter and system-headers, and 
similarly handled in IO.mapOptional.
- It's not PCH.
- It uses set/getTraversalScope as an implementation method, but the goal of 
skip-headers is not to limit clang-tidy to see only non-header Decls.
- Improvements on PPCallback-based and static analyzer checks could be in 
follow up changes. The gain might not be as large as the MatchFinder-based 
checks and the implementation could be more complicated than using 
set/getTraversalScope.

I am glad that with your help on set/getTraversalScope, 
many part of this revision has become much simpler.
I am looking forward to applying this new feature, even with only part 1, 
to save very significant Android build time, but I am also extremely cautious
not to combine many risky changes into one big revision.


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