Page MenuHomePhabricator

[clang-tidy] New feature --skip-headers, part 1, LocFilter
Needs ReviewPublic

Authored by chh on Mar 16 2021, 7:22 AM.

Details

Summary

[clang-tidy] New feature --skip-headers, LocFilter

  • This new feature has impacts similar to --header-filter and --system-headers.
    • Clang-tidy by default checks header files, reports the number of warnings found in header files but not those warning messages.
    • With --header-filter, clang-tidy shows warnings in selected header files, but not system header files.
    • With --system-headers, clang-tidy shows also warnings in system header files.
    • With --skip-headers, clang-tidy does not check header files, and does not know or count warnings in header files. However, if --header-filter or --system-headers are used, those header files will be checked and warnings be counted and displayed.
  • For users who do not need to know the number of not-shown warnings in header files, this feature saves maybe 60% to 80% of clang-tidy run-time, by skipping checks of header files. When building Android system with clang-tidy, repeated checks on commonly included header files in multiple compilation units took significant part of build time, which could be saved by this new feature.
  • Note that this is not the same as PCH, which usually saves parsing time, but not semantic checks or code generation time.
  • This part 1 implementation only affects the MatchFinder-based checks, not the PPCallback-based or the static analyzer checks. Follow up changes could make the other checks to skip header files and save more time.
  • 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.
  • Implementation Methods:
    • Add a shared LocFilter to Tidy's MatchFinderOptions.
    • In MatchASTVisitor::TraverseDecl, skip a DeclNode if the MatchFinderOptions has a LocFilter that decides to skip the DeclNode location.
    • LocFilter::skipLocation checks if a location should not be checked.
    • Use simple cache of the last skipped/accepted FileID in LocFilter to minimize the overhead of skipLocation.
    • Add an AllFileFinder for checks that need to match all source files.
      • ForwardDeclarationNamespaceCheck is registered as an AllFileCheck, to avoid missing valid warnings on main source files.
      • UnusedUsingDeclsCheck is registered as an AllFileCheck, to avoid incorrect warnings on main source files.
      • Their diag functions call ClangTidyCheck::skipLocation to skip diagnostics on header files.
  • Tests:
    • Add skip-headers*.cpp tests to show the effects of new flags.
    • Add bugprone-forward-declaration-namespace-header.cpp test to show how AllFileMatchFinder-based checks can depend on header Decls.
    • Add new test cases into misc-unused-using-decls.cpp.
    • Extend some clang-tidy checker tests to make sure that --skip-headers skips checks/warnings in the header files: google-namespaces.cpp modernize-pass-by-value-header.cpp
    • Add runs with --skip-headers in some clang-tidy checker tests to make sure that the option does not hide warnings in the main file:
      • about 18 other clang-tools-extra/test/clang-tidy/checkers/*.cpp
    • Some tests were added --skip-headers=0 flag to get expected warning messages in test header files: checkers/abseil-no-internal-dependencies.cpp checkers/abseil-upgrade-duration-conversions.cpp checkers/google-namespaces.cpp infrastructure/file-filter-symlinks.cpp infrastructure/file-filter.cpp infrastructure/line-filter.cpp

Diff Detail

Event Timeline

chh created this revision.Mar 16 2021, 7:22 AM
chh requested review of this revision.Mar 16 2021, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 7:22 AM
chh updated this revision to Diff 339065.Apr 20 2021, 6:12 PM

hide the --show-all-warnings flag from --help

chh updated this revision to Diff 351617.Jun 11 2021, 6:40 PM
chh retitled this revision from clang-tidy skip-headers; Alt#1; fewer changes to [clang-tidy] Add --skip-headers, part 1.
chh edited the summary of this revision. (Show Details)

sync with latest clang/llvm

chh updated this revision to Diff 359271.Fri, Jul 16, 3:22 AM
chh retitled this revision from [clang-tidy] Add --skip-headers, part 1 to [clang-tidy] New feature --skip-headers, (tested as default).
chh edited the summary of this revision. (Show Details)

sync with latest source, update more tests to test skip-headers as default

chh updated this revision to Diff 359461.Fri, Jul 16, 3:18 PM
chh edited the summary of this revision. (Show Details)

only coding style changes in clang-tidy files, to match other alternative implementation

chh updated this revision to Diff 359531.Fri, Jul 16, 10:43 PM
chh retitled this revision from [clang-tidy] New feature --skip-headers, (tested as default) to [clang-tidy] New feature --skip-headers, part 1, skip Decls.
chh edited the summary of this revision. (Show Details)

--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.

chh updated this revision to Diff 360336.Tue, Jul 20, 6:39 PM
chh edited the summary of this revision. (Show Details)

Add bugprone-forward-declaration-namespace-header.cpp test; fix some coding style warnings.

chh updated this revision to Diff 360649.Wed, Jul 21, 5:01 PM
chh edited the summary of this revision. (Show Details)

Add one more skip-headers-2.cpp test.

chh updated this revision to Diff 361295.Fri, Jul 23, 12:17 PM
chh retitled this revision from [clang-tidy] New feature --skip-headers, part 1, skip Decls to [clang-tidy] New feature --skip-headers, part 1, LocFilter.
chh edited the summary of this revision. (Show Details)

Add an AllFileFinder for checks that need to match all source files.
ForwardDeclarationNamespaceCheck is registered as an AllFileCheck,

chh updated this revision to Diff 361870.Mon, Jul 26, 6:00 PM

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

chh updated this revision to Diff 362216.Tue, Jul 27, 4:15 PM
chh edited the summary of this revision. (Show Details)
  • Use clang-format layout in ClangTidy.cpp and ClangTidyModule.h.
  • Fix misc-unused-using-decls false-positive warning from --skip-headers.
    • UnusedUsingDeclsCheck is now registered as an AllFileCheck.
    • Add new test cases into misc-unused-using-decls.cpp.
chh updated this revision to Diff 362262.Tue, Jul 27, 7:09 PM

apply clang-format

chh updated this revision to Diff 362702.Thu, Jul 29, 3:37 AM

sync to the latest source; apply clang-format; add new skip-headers-4.cpp test;
skip modernize-use-nullptr warnings in header files in UseNullptrCheck.cpp

This is enhanced implementation over D98710, to handle the new found failed test cases.