[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