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

[chh]

Summary

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

• This depends on https://reviews.llvm.org/D104071.
• 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:
  • Depending on new changes to set/getTraversalScope in https://reviews.llvm.org/D104071, the AST was modified to exclude header file Decls during the call to MatchFinder's ASTConsumer's HandleTranslationUnit.
    • Note that this could impact a MatchFinder-based check, if it is performed on a non-header file, but needs some Decl in headers. Luckily, we haven't seen such a case. If there is such a case in the future, we might need some other method to skip checks without cutting out Decls in AST.
    • Note that this method does not apply to PPCallback-based checks, as many of those checks could give a warning on one file but depend on Decl or macros in other header files. Similarly static analysis checks usually depend on calls to functions in the header files. Not seeing header file Decl in AST could affect such checks on non-header files.
  • ClangTidyASTConsumer overrides two member functions of MultiplexConsumer and accesses the Consumers data member.
    • in HandleTopLevelDecl to collect filtered Decls.
    • in HandleTranslationUnit to call get/setTraversalScope before calling the Finder ASTConcumser, which calls matchAST.
  • DeclFilter::skipDecl checks/skips top level Decls.
  • Use simple cache of the last skipped/accepted FileID in DeclFilter to minimize the overhead of skipLocation.
• Tests:
  • Add skip-headers*.cpp tests to show the effects of new flags. Some tests need to skip nested Decls and Stmts, even if their top-level Decls should not be skipped.
  • Add bugprone-forward-declaration-namespace-header.cpp test to show that a MatchFinder-based check needs to see all header Decls to issue valid warnings on the main file.
  • Add test cases into misc-unused-using-decls.cpp to show that a MatchFinder-based check needs to see all header Decls to avoid issuing invalid warnings on the main file.
  • 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

[chh]

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

[chh]

sync with latest clang/llvm

[chh]

Friendly ping reviewers. Now this change can pass all clang-tidy tests.
Please feel free to comment and/or add other reviewers. Thanks.

[sammccall]

This comment was removed by sammccall.

[sammccall]

Sorry, previous sent too soon. +hokein as I'll be out next week.

I think we should be careful about which layers the filtering logic is added to, and how much surface area it has.

The minimal thing seems to be adding the clang-tidy options, and modifying the AST-consumer bits of the clang-tidy driver to compute and set the traversal scope based on these options.
I don't think we should bake the "filter" concept into other layers in clang unless there's a strong reason we need to do so.
(As discussed offline, providing a "filter" API requires all the candidates to be enumerated, which can be an expensive operation in the presence of modules/PCH - that's why the model in common layers today is "optional list of top-level decls that should be traversed").

clang-tools-extra/clang-tidy/ClangTidyCheck.h

31 ↗

(On Diff #351616)

The layering in clang-tidy is... not particularly great or clear, but there is a distinction between driver-y type code (the clang-tidy application) vs the checks themselves.

As I understand our plan, this filter is "just" a way to configure the traversal scope before running, i.e. what information the AST will expose to the checks.
That is, this is a driver concern and should not be visible to the checks - probably should go in ClangTidy.h or even just ClangTidy.cpp as part of the ClangTidyASTConsumer. My litmus test here is that we do have other drivers (at least clangd that I know of), and they shouldn't be seeing/reusing this mechanism, and so neither should checks.

(Similarly if we do something with PP in the future, hopefully this can *also* be transparent to checks)

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

I don't think the filter belongs here.
The design of traversal scope is that it's a property of the AST that affects all traversals, so it shouldn't be a configuration property of particular traversals like ASTMatchFinder.

There's a question of what to do about MatchFinder::newASTConsumer(), which runs the finder immediately after an AST is created, and so covers the point at which we might set a scope. There are several good options, e.g.:

• Don't provide any facility for setting traversal scope when newASTConsumer() is used - it's not commonly needed, and the ASTConsumer is trivial to reimplement where that's actually needed
• Accept an optional function<void(ASTContext&)> which should run just before matching starts

This seems a bit subtle, but the difference between having this in MatchFinder proper vs just in newASTConsumer() is important I think, precisely because it's common to run the matchers directly on an existing AST.

[chh]

Sync up; moved DeclFilter to ClangTidy.cpp.

[chh]

clang-tools-extra/clang-tidy/ClangTidyCheck.h

31 ↗

(On Diff #351616)

Thanks. Moved this class to ClangTidy.cpp.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

I have some similar concerns too.

clangd calls MatchFinder::matchAST explicitly after setTraversalScope,
but clang-tidy uses MultiplexConsumer and MatchFinder::newASTConsumer
is just one of the two consumers.
(1) I am not sure if it would be safe or even work to make big changes in
ClangTidy.cpp's CreateASTConsumer to call MatchFinder::matchAST explicitly.
(2) Similarly, I wonder if setTraversalScope will work for both MatchFinder
and other consumers in the same MultiplexConsumer.

Could you check if my current change to MatchFinder::HandleTranslationUnit
is right, especially in the saving/restoring of traversal scope?

I am assuming that each consumer in a MultiplexConsumer will have its own
chance to HandleTranslationUnit and HandleTopLevelDecl.
If that's correct, it seems to me that changing those two handlers in
MatchFinder is right for clang-tidy. Then they will need the optional
MatchFinder::MatchFinderOptions::DeclFilter.

[hokein]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

but clang-tidy uses MultiplexConsumer and MatchFinder::newASTConsumer is just one of the two consumers.

Yeah, MultiplexConsumer is a separate interface in clang, and I don't think we have a strong reason to modify it.

However, we could refine the bit in clang-tidy -- clang-tidy uses MultiplexConsumer to dispatch all events to two consumers: the MatcherFinder, the clang's static analyzer, we can get rid of the MultiplexConsumer by implementing a new ASTConsumer, so that we have enough flexibility to affect all traversals without touching all clang areas, so the API will look like

class ClangTidyASTConsumer : public ASTConsumer {

public:
  void HandleTopLevelDecl(...) override {
     // collect all main file decl
  }
  void HandleTranslationUnit(ASTContext &Context) override {
    // set traversal scope.
    MatcherFinderConsumer->HandleTranslationUnit(Context);
    StaticAnalyzer->HandleTranslationUnit(Context);
  }
  
  // implement other necessary Handle* overrides, and dispatch to StaticAnalyzer consumers;

private:
  std::unique_ptr<ASTConsumer> MatcherFinderConsumer;
  std::unique_ptr<...> StaticAnalyzer;
  ... MainFileDecl;
};

[sammccall]

Back from vacation!

clang-tools-extra/clang-tidy/ClangTidy.cpp

461

this is probably worth some explanation

472

this comment didn't match the next line.
Did you intend to check Location.isFile() instead?

475

maybe add a comment explaning why the expansion loc is chosen?

475

note that this means we're going to traverse up the macro expansion edges even if we're hitting the cache. You may want to cache under the original file ID instead?

477

caching all results in a DenseMap<FileID, bool> seems likely to perform better, no?

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

MultiplexConsumer is just used as a helper to glue together two concrete ASTConsumers, not arbitrary consumers, so we can reason about whether it's safe by reading the code.

The static analyzer's consumer appears to already track top-level decls rather than traversing from the root, exactly to avoid traversing stuff from the preamble. So I would expect setTraversalContext to work fine there.

The approach Haojian suggests of avoiding MultiplexConsumer, and making the tight coupling to the particular consumers explicit, also seems fine.

If that's correct, it seems to me that changing those two handlers in MatchFinder is right for clang-tidy

This might be the most convenient thing for clang-tidy, but I don't think it's the right thing for MatchFinder, which is also important.

[chh]

update some comments and sync again with the latest clang/llvm source

[chh]

clang-tools-extra/clang-tidy/ClangTidy.cpp

472

I just wanted to make sure that we can get valid FileID.
Updated the comment.

475

This is to match the ClangTidyDiagnosticConsumer::checkFilters.
Do you think some other get*Location method should be used in both places?

475

I would rather explore various cache performance improvements in follow up CLs.
In my performance tests, current simple check of LastAcceptedFileID and LastSkippedFileID already has very high hit rate due to sequential check of Decls in the included header files. More caching improvements should produce relatively smaller difference, which will be shown/visible easier in their own small change CLs.

477

I am sure most calls hit the last FileID. For the missed calls, I was planning to add other caching mechanism in skipFileID function. That could be in follow up CLs with more accurate measurements to compare the overhead vs hit rate.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

I looked into Hokein's suggestion. Although in general correctly used inheritance is better, there are special cases that copying functionality from another class could be better. I wondered if it could work better by changing ClangTidyASTConsumer to behave like MultiplexConsumer without inheritance.

After looking into MultiplexConsumer's many methods, what are used and could be used in the future by MatchFinder's MatchASTConsumer and static analyzer's AnalysisConsumer, the dependency on MultiplexConsumer to pass through all Consumer methods is obvious. So, ClangTidyASTConsumer really needs to behave like MultiplexConsumer, duplicating all existing and future methods, to keep MatchFinder and AnalysisConsumer working. This looks like a real inheritance use case and difficult to make it better without inheritance.

If MatchFinder users really don't want an optional filter for HandleTranslationUnit and HandleTopLevelDecl, I think an alternative is to define a child class maybe called TidyMatchFinder with the extra features needed now only by clang-tidy. But, there is also advantage in adding these into MatchFinder now for future non-clang-tidy users. That will be similar to the current optional MatchFinder's CheckProfiling and the set/getTraversalScope. Those features/interfaces started with one user/tool and now we see more users/tools. If they were defined in a child class specific to clangd or some profiler, we will now need to rename or move those features to a common base class to share.

Please feel free to suggest any alternative, or let me know if you are okay with current optional filter in MatchFinder, or if you insist on defining a new child class like TidyMatchFinder.
Thanks.

[sammccall]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

148 ↗

(On Diff #351616)

TL;DR is I think this is a clang-tidy feature that should not be visible in the matcher layer in any way.
If it's going to touch ASTMatcher I think we should have a strong design reason or a very strong implementation reason. That it's kind of awkward to code and might one day be useful isn't sufficient.

There are four layers here: AST nodes themselves, RecursiveASTVisitor (which defines a tree via parent/child relationships), the matcher framework, and finally clang-tidy.
The mechanics of filtering using a list are solved in RecursiveASTVisitor.
There seem to be two issues here against solving the rest in the clang-tidy layer.

---

First is conceptual: is having filter APIs in the matcher framework good? I don't think so, because:

• it's duplicative: it mirrors the setTraversalScope functionality and it's unclear when to use which. Having two ways to accomplish a rare task seems excessive.
• it conflicts with the design of this layer: matchers are all about non-destructive reading, but setting traversal scope has side effects. So it's only really usable with the ASTConsumer, which is a corner of the interface.
• it's not necessary: it's always possible to get at the underlying ASTContext wherever a MatchFinder will be used.

You suggest this might be good for "future non-clang-tidy users" but I don't think we should add it speculatively for this purpose. It seems at least as likely that such users will never materialize, or that the solution that is right for clang-tidy is not right for them.

---

Second is practical. That ClangTidyASTConsumerFactory::CreateASTConsumer needs to dispatch both to MatchFinder and the static analyzer's (non-trivial) ASTConsumer. We have to add a call to setTraversalScope somewhere, and none of the approaches are perfectly natural.

However there are several that are OK:

• subclass MultiplexingASTConsumer to inject the call in HandleTranslationUnit
• subclass MultiplexingASTConsumer and inject the call and also the matcher-based-checks logic in HandleTranslationUnit (so MAC delegates only to the static analyzer)
• write an ASTConsumer without MultiplexingASTConsumer that manually forwards only the actually needed methods to the static analyzer consumer
• write an unrelated ASTConsumer that forwards every method to the static analyzer consumer
• refactor static analyzer to expose its consumer class, and subclass that (yuck!)

These do require making *some* assumptions about how these consumers work. But that's OK - there are two of them, not thousands, and these interfaces change rarely. We don't need to architect defensively against any possible future change, and that's not culturally how LLVM operates either. I'd lean towards e.g. the first option, which requires fewer assumptions.

[chh]

Move almost all changes to MatchFinder into ClangTidy.cpp.

[chh]

I reproduced the ClangdTests::TargetDeclTest.Concept failure locally without this change,
using llvm git commit 0c400e8953. Will wait for upstream fix of the failed tests.

[chh]

with clang-format suggested change

[sammccall]

Thanks, this looks much closer.

Customizing the MatchFinder::matchAST() to set the scope is a neat try but I don't think the design really fits (and we have to abuse inheritance to make it work).
Ultimately you need to customize both near HandleTranslationUnit and near HandleTopLevelDecl, and especially the latter really is an ASTConsumer concern.
So implementing an ASTConsumer in some form seems necessary.

I reproduced the ClangdTests::TargetDeclTest.Concept failure locally without this change,

I'll have a look, can't reproduce it locally. Have you seen it on any buildbots?

clang-tools-extra/clang-tidy/ClangTidy.cpp

475

Do you think some other get*Location method should be used in both places?

It depends on what you want to achieve. Matching the check-filters behavior elsewhere makes sense.
(It does raise the question though why the code can't be shared, even if the line filter doesn't apply per se)

475

Is restoring the traversal scope strictly needed by the static analyzer? I would expect not, but I might be wrong.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

144 ↗

(On Diff #355827)

Matchfinder specifically says "not intended to be subclassed".

147 ↗

(On Diff #355827)

This function doesn't make sense at this layer - calling it does nothing and the class is intended to be used directly, not to be subclassed. And MatchFinder itself doesn't "consume" top-level decls - that's an ASTConsumer concern.

I understand you use it here as a hook to listen for decls seen by the MatchFinder::newASTConsumer().
Better would be to pass a HandleTopLevelDecl callback to newASTConsumer() instead.
But given that MatchFinderASTConsumer is completely trivial and can be built on the public MatchFinder API, better still would just be to reimplement it with the unusual functionality ClangTidy needs.

[sammccall]

In D98710#2852645, @sammccall wrote:

I reproduced the ClangdTests::TargetDeclTest.Concept failure locally without this change,

I'll have a look, can't reproduce it locally. Have you seen it on any buildbots?

I can't find it on any buildbots or the premerge tests, and can't see it locally either at the commit you suggest nor with this patch applied.
So there might be something going on, but it seems unrelated for sure.

[chh]

Last build tests were green. Let's see if the new updated diff still passes all the tests.

Looks like all choices have some risk/overhead in future maintenance and we need to pick one with the least cost.
I think cloning classes is most expensive and risky to keep future compatibility.
Adding a new subclass or optional members into existing class are more conventional solutions.

clang-tools-extra/clang-tidy/ClangTidy.cpp

475

I think it is safer to assume low or acceptable overhead in get/setTraversalScope and keep the impact only to the MatchFinder consumer, rather than depending on static analyzer working now or in the future with the changed AST.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

144 ↗

(On Diff #355827)

Do you know the reason it cannot have subclass?
All our current implementation choices need to change its matchAST behavior, either by overriding it, or changing the AST before it, or skip Decl in the recursive walk of AST.
For any reason not to have subclass, changing AST outside matchAST seems riskier than changing inside matchAST in a child class.

147 ↗

(On Diff #355827)

Please review the updated diff. Now HandleTopLevelDecl is all in clang-tidy.
For both the MultiplexConsumer and MatchFinder, I think it is more feasible to resolve all subclass issues now and maintain the inheritance relation than making clone implementations and trying to maintain the same behavior in the future.

[chh]

fix clang-format name style issue

[chh]

fix clang-format coding style issue

[chh]

No more change to or derivation from MatchFinder, but override two member functions in MultiplexConsumer.

[sammccall]

This generally looks good now, mostly nits left.

The exception is the remaining complexity around stashing and restoring the traversal scope state.
If it's needed, it points to a deeper problem in static analyzer and we should understand it a bit (and whether it must be fixed as it will undo the performance characteristics of this feature in some circumstances).

clang-tools-extra/clang-tidy/ClangTidy.cpp

310

Ctx is ultimately unused, just take the regex instead?

346

please find a different name for this function or merge the two together, having two different functions differing only by case is confusing

348

this function isn't needed - it's a trivial accessor called once in a place where we can access the field directly.
Similarly with hasFilter. The code accessing the fields directly reads just as clearly.

428

can you move the impl of the members next to the class?

475

I think it is safer to assume low or acceptable overhead in get/setTraversalScope

We have reasons to believe this is not cheap. The first usage of getParent() after changing traversal scope incurs a full AST traversal.

rather than depending on static analyzer working now or in the future with the changed AST.

There are a fairly small number of reasons that setTraversalScope could break static analyzer at this point, and I think they're all going to cause the static analyzer to be slow when used with PCHes. However the static analyzer's implementation and comments say that it goes to effort to be fast with PCHes. So I do believe this is a more stable assumption, and in the long run we may be able to simplify its implementation.

514

This adds significant conceptual complexity and possible runtime costs to guard against the possibility that the static analyzer would not be compatible with simply setting the traversal scope.

Please do not do this. It may appear that you're cheaply eliminating a class of bugs, but complexity is not cheap, and in the long run can also introduce bugs. Instead investigate whether this is a real problem and why.

If it's not then Context.setTraversalScope() + MultiplexingASTConsumer::HandleTranslationUnit is enough here, and FinderASTConsumerPtr can go away.

608

move SharedDeclFilter

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

99

Maybe an explicit comment that ShowAllHeaders, SkipHeaders are runtime/optimization options that are configured at the command-line only, and therefore not mapped here

clang-tools-extra/clang-tidy/ClangTidyOptions.h

79

This overrides (and is therefore grouped with) HeaderFilterRegex, SystemHeaders.
So the order should probably be
HeaderFilterRegex
SystemHeaders
ShowAllWarnings
SkipHeaders

Comment should comment should explicitly say both "This overrides HeaderFilterRegex, ..." and "This is an option intended for testing/debugging clang-tidy".

84

This is easy to confuse with e.g. HeaderFilterRegex and SystemHeaders, which control policy rather than just implementation strategy.
I'd suggest calling this something like PruneTraversal which hints at the implementation strategy it controls but most importantly that it's *not* about whether you want warnings from the headers or not.

(similarly for the flag)

clang/include/clang/ASTMatchers/ASTMatchFinder.h

144 ↗

(On Diff #355827)

Do you know the reason it cannot have subclass?

Because widely-used classes need to decide whether to accept the maintenance cost of supporting inheritance or not, and the author judged that the (conceptual) complexity of supporting it outweighs the things enabled by doing so.

[chh]

sync up latest clang/llvm source; more format/style changes

[chh]

clang-tools-extra/clang-tidy/ClangTidy.cpp

310

Context is used in skipFileID.

475

Okay, won't assume low overhead in set/getTraversalScope.
So far their impact is limited to skip-headers, which in this step is limited to only MatchFinder. Please see also my reply in the other comment.

514

For this first step implementation of skip-headers, could we limit the risk and impact to only AST MatchFinder based checks? If it works fine, we can add more performance improvements, handle PPCallback-based checks, and static analyzer checks. We can turn skip-headers to default or revert any follow up step.

At this time, we only know that PPCallback-based checks can save some more time with skip-headers, but do not know if any static analyzer check can benefit from skip-headers easily. In other words, we are sure to deliver big saving of runtime for clang-tidy users that do not enable static analyzer checks, but not sure about static analyzer check users.

The overhead and complexity of set/getTraversalScope and checking filters will be on the users of skip-header, but not on the static analyzer checkers.

If we apply the same change to AST for all consumers, the saving of code complexity is smaller than the risk of impact to static analyzer checks, and the saving of runtime is also much smaller than the already saved time in MatchFinder.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

99

Why SystemHeaders is not mapped?
I agree that ShowAllWarnings is not important here,
but SkipHeaders seems as important to be included here as HeaderFilterRegex and SystemHeaders.

clang-tools-extra/clang-tidy/ClangTidyOptions.h

84

Are you suggesting a change of flag name from --skip-headers to --prune-traversal and internal option name from SkipHeaders to PruneTraversal?

So far, "traversal" seems to be an implementation term not yet used in clang-tidy flags. But there is TraversalKind used in some clang-tidy checks. They seem to mean different things from HeaderFilterRegex, and SystemHeaders.

The confusion exists between HeaderFilterRegex and SystemHeaders. Both should really do skip-check-of-headers. Their "implementation" is like a short-cut to only suppress output but not skipping the checks. When skip-headers is enabled and maybe become the default, both HeaderFilterRegex and SystemHeaders will make more sense.

[sammccall]

clang-tools-extra/clang-tidy/ClangTidy.cpp

310

Oops, I did miss this. Since we extract the header regex, can we also pull out the one boolean that we check later into a field? This would make it clearer how the context is used, since it's a large object.

514

OK, fair enough. However you seem to both be saying "let's do this for now" and "if traversal scope is unimportant for static-analyzer performance, we need never change this".
The main point of applying traversal scope uniformly is to avoid complexity and special cases, not to improve performance.

If we want to have this fairly unusual mechanism and require maintainers to understand it, then needs to be mitigated by a significant comment somewhere explaining:

• what is being done (wrapping HandleTranslationUnitDecl in traversalscope load/restore logic for one consumer only)
• how it's being done (keep an extra pointer to the "special" consumer so we can identify it)
• why it's being done (ideally concrete problems in applying the scope everywhere, but at least why we don't expect this to be safe)

Coming up with a good answer to the "why" question is part of this change.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

99

Why SystemHeaders is not mapped?

No idea, sorry.

but SkipHeaders seems as important to be included here as HeaderFilterRegex and SystemHeaders.

I don't think so.

HeaderFilterRegex and SystemHeaders control policy, and it's reasonable for that to depend on the specific code (and therefore be dynamically configurable).
SkipHeaders only controls the mechanism/implementation strategy - why do we need to support different strategies for different files in the same clang-tidy invocation, rather than just rolling it out with a flag?

Moreover, the plan AIUI is for SkipHeaders to become the default and for the option to go away. We should try to avoid supporting it in config files if that's the case.

clang-tools-extra/clang-tidy/ClangTidyOptions.h

84

Are you suggesting a change of flag name from --skip-headers to --prune-traversal and internal option name from SkipHeaders to PruneTraversal?

Yes, sorry I left the comment at the wrong spot.

So far, "traversal" seems to be an implementation term not yet used in clang-tidy flags.

That's right, because clang-tidy flags generally control behavior rather than implementation. But "skip headers" is different - it's intended to produce the same set of warnings, but faster by doing the filtering at a different step. In the long run I would prefer not to have this flag, but if we need it then it should have a name that emphasizes the implementation strategy, which is what it controls.

But there is TraversalKind used in some clang-tidy checks. They seem to mean different things from HeaderFilterRegex, and SystemHeaders.

Yes, TraversalKind is unrelated (and IMO also poorly named).

The confusion exists between HeaderFilterRegex and SystemHeaders. Both should really do skip-check-of-headers. Their "implementation" is like a short-cut to only suppress output but not skipping the checks. When skip-headers is enabled and maybe become the default, both HeaderFilterRegex and SystemHeaders will make more sense.

Sorry, I don't understand this paragraph. HeaderFilterRegex and SystemHeaders both help define which files are "in scope" for clang-tidy checks. This is the same whether --skip-headers=1 or --skip-headers=0, the only difference is at what stage the filtering is applied. I agree the --show-all-warnings flag should disable post-filtering for both.

[chh]

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.

[sammccall]

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.

[chh]

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

[chh]

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

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.

[chh]

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

[sammccall]

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.

[chh]

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.

[chh]

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.

[chh]

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

[chh]

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

[chh]

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

clang-tools-extra/clang-tidy/ClangTidy.cpp

private:
   llvm::StringMap<Replacements> FileReplacements;
   ClangTidyContext &Context;
   FixBehaviour ApplyFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
   unsigned WarningsAsErrors;
 };
 
+// Check if a Decl should be skipped by clang-tidy checks.
+struct DeclFilter {
+  DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
+      : Context(Ctx), Sources(SM) {
+    HeaderFilter =
+        std::make_unique<llvm::Regex>(*Ctx.getOptions().HeaderFilterRegex);
+    LastSkippedFileID = FileID::getSentinel();
+    LastAcceptedFileID = FileID::getSentinel();
+  }
+  bool skipDecl(Decl *);
+  bool skipLocation(SourceLocation);
+
+private:
+  bool skipFileID(SourceLocation, FileID);
+  ClangTidyContext &Context;
+  SourceManager &Sources;
+  std::unique_ptr<llvm::Regex> HeaderFilter;
+  FileID LastSkippedFileID;
+  FileID LastAcceptedFileID;
+};
+
 class ClangTidyASTConsumer : public MultiplexConsumer {
 public:
   ClangTidyASTConsumer(std::vector<std::unique_ptr<ASTConsumer>> Consumers,
                        std::unique_ptr<ClangTidyProfiling> Profiling,
                        std::unique_ptr<ast_matchers::MatchFinder> Finder,
+                       ASTConsumer *FinderASTConsumerPtr,
+                       std::shared_ptr<DeclFilter> Filter,
                        std::vector<std::unique_ptr<ClangTidyCheck>> Checks)
       : MultiplexConsumer(std::move(Consumers)),
         Profiling(std::move(Profiling)), Finder(std::move(Finder)),
-        Checks(std::move(Checks)) {}
+        Checks(std::move(Checks)), FinderASTConsumerPtr(FinderASTConsumerPtr),
+        Filter(Filter) {}
+  ~ClangTidyASTConsumer() override;
+  bool HandleTopLevelDecl(DeclGroupRef DG) override;
+  void HandleTranslationUnit(ASTContext &Context) override;
 
 private:
+  bool handleTopLevelDecl(DeclGroupRef DG);
+  bool hasFilter() { return Filter.get() != nullptr; }
+  std::vector<Decl *> &getTraversalScope() { return TopLevelDecls; }
+
   // Destructor order matters! Profiling must be destructed last.
   // Or at least after Finder.
   std::unique_ptr<ClangTidyProfiling> Profiling;
   std::unique_ptr<ast_matchers::MatchFinder> Finder;
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
+  /// Remembered Finder's ASTConsumer, which is also pushed into
+  /// and managed by MultiplexConsumer.
+  ASTConsumer *FinderASTConsumerPtr;
+
+  // When --skip-headers, Filter is not null and handleTopLevelDecl
+  // will collect Decls into TopLevelDecls.
+  /// Check if a Decl should be skipped.
+  std::shared_ptr<DeclFilter> Filter;
+  /// All (filtered) top level decls.
+  std::vector<Decl *> TopLevelDecls;
 };
 
 } // namespace
 
 ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
     ClangTidyContext &Context,
     IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS)
     : Context(Context), OverlayFS(std::move(OverlayFS)),
if (CheckName.startswith("core") ||
         Context.isCheckEnabled(ClangTidyCheckName)) {
       List.emplace_back(std::string(CheckName), true);
     }
   }
   return List;
 }
 #endif // CLANG_TIDY_ENABLE_STATIC_ANALYZER
 
+bool DeclFilter::skipFileID(SourceLocation Location, FileID FID) {
+  // do not skip the main file
+  if (Sources.isInMainFile(Location))
+    return false;
+  // skip system headers unless --system-headers is used
+  if (Sources.isInSystemHeader(Location))
+    return !*Context.getOptions().SystemHeaders;
+  const FileEntry *File = Sources.getFileEntryForID(FID);
+  // without a FileEntry, cannot verify header file path to skip
+  if (!File)
+    return false;
+  StringRef FileName(File->getName());
+  // skip, unless file name matches --header-filter
+  return !HeaderFilter->match(FileName);
+}
+
+bool DeclFilter::skipDecl(Decl *Decl) {
+  return skipLocation(Decl->getLocation());
+}
+
+bool DeclFilter::skipLocation(SourceLocation Location) {
+  // cannot get valid FileID if Location is invalid
+  if (!Location.isValid())
+    return false;
+  // Similar to ClangTidyDiagnosticConsumer::checkFilters,
+  // the decision to skip a check depends on getDecomposedExpansionLoc.
+  auto FID = Sources.getDecomposedExpansionLoc(Location).first;
+  // Quick check against last checked results.
+  if (FID == LastSkippedFileID)
+    return true;
+  if (FID == LastAcceptedFileID)
+    return false;
+  auto ShouldSkip = skipFileID(Location, FID);
+  if (ShouldSkip) {
+    LastSkippedFileID = FID;
+  } else {
+    LastAcceptedFileID = FID;
+  }
+  return ShouldSkip;
+}
+
+template <class T>
+bool isTemplateSpecializationKind(const NamedDecl *D,
+                                  TemplateSpecializationKind Kind) {
+  if (const auto *TD = dyn_cast<T>(D))
+    return TD->getTemplateSpecializationKind() == Kind;
+  return false;
+}
+
+bool isTemplateSpecializationKind(const NamedDecl *D,
+                                  TemplateSpecializationKind Kind) {
+  return isTemplateSpecializationKind<FunctionDecl>(D, Kind) ||
+         isTemplateSpecializationKind<CXXRecordDecl>(D, Kind) ||
+         isTemplateSpecializationKind<VarDecl>(D, Kind);
+}
+
+bool isImplicitTemplateInstantiation(const NamedDecl *D) {
+  return isTemplateSpecializationKind(D, TSK_ImplicitInstantiation);
+}
+
+ClangTidyASTConsumer::~ClangTidyASTConsumer() {}
+
+bool ClangTidyASTConsumer::HandleTopLevelDecl(DeclGroupRef DG) {
+  return MultiplexConsumer::HandleTopLevelDecl(DG) && handleTopLevelDecl(DG);
+}
+
+bool ClangTidyASTConsumer::handleTopLevelDecl(DeclGroupRef DG) {
+  if (hasFilter()) {
+    for (Decl *D : DG) {
+      if (const NamedDecl *ND = dyn_cast<NamedDecl>(D))
+        if (isImplicitTemplateInstantiation(ND))
+          continue;
+
+      // ObjCMethodDecl are not actually top-level decls.
+      if (isa<ObjCMethodDecl>(D))
+        continue;
+
+      if (!Filter->skipDecl(D))
+        TopLevelDecls.push_back(D);
+    }
+  }
+  return true;
+}
+
+void ClangTidyASTConsumer::HandleTranslationUnit(ASTContext &Context) {
+  for (auto &Consumer : Consumers) {
+    if (hasFilter() && Consumer.get() == FinderASTConsumerPtr) {
+      // Modify AST before calling the Finder's ASTConsumer.
+      auto SavedScope = Context.getTraversalScope();
+      Context.setTraversalScope(getTraversalScope());
+      Consumer->HandleTranslationUnit(Context);
+      Context.setTraversalScope(SavedScope);
+    } else {
+      Consumer->HandleTranslationUnit(Context);
+    }
+  }
+}
+
 std::unique_ptr<clang::ASTConsumer>
 ClangTidyASTConsumerFactory::CreateASTConsumer(
     clang::CompilerInstance &Compiler, StringRef File) {
   // FIXME: Move this to a separate method, so that CreateASTConsumer doesn't
   // modify Compiler.
   SourceManager *SM = &Compiler.getSourceManager();
   Context.setSourceManager(SM);
   Context.setCurrentFile(File);
ClangTidyASTConsumerFactory::CreateASTConsumer(
   ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
 
   std::unique_ptr<ClangTidyProfiling> Profiling;
   if (Context.getEnableProfiling()) {
     Profiling = std::make_unique<ClangTidyProfiling>(
         Context.getProfileStorageParams());
     FinderOptions.CheckProfiling.emplace(Profiling->Records);
   }
+  std::shared_ptr<DeclFilter> SharedDeclFilter;
+  if (*Context.getOptions().SkipHeaders)
+    SharedDeclFilter =
+        std::shared_ptr<DeclFilter>(new DeclFilter(Context, *SM));
 
   std::unique_ptr<ast_matchers::MatchFinder> Finder(
       new ast_matchers::MatchFinder(std::move(FinderOptions)));
 
   Preprocessor *PP = &Compiler.getPreprocessor();
   Preprocessor *ModuleExpanderPP = PP;
 
   if (Context.getLangOpts().Modules && OverlayFS != nullptr) {
     auto ModuleExpander = std::make_unique<ExpandModularHeadersPPCallbacks>(
         &Compiler, OverlayFS);
     ModuleExpanderPP = ModuleExpander->getPreprocessor();
     PP->addPPCallbacks(std::move(ModuleExpander));
   }
 
   for (auto &Check : Checks) {
     Check->registerMatchers(&*Finder);
     Check->registerPPCallbacks(*SM, PP, ModuleExpanderPP);
   }
 
   std::vector<std::unique_ptr<ASTConsumer>> Consumers;
-  if (!Checks.empty())
-    Consumers.push_back(Finder->newASTConsumer());
+  ASTConsumer *FinderASTConsumerPtr = nullptr;
+  if (!Checks.empty()) {
+    std::unique_ptr<ASTConsumer> FinderASTConsumer(Finder->newASTConsumer());
+    FinderASTConsumerPtr = FinderASTConsumer.get();
+    Consumers.push_back(std::move(FinderASTConsumer));
+  }
 
 #if CLANG_TIDY_ENABLE_STATIC_ANALYZER
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
   AnalyzerOptions->CheckersAndPackages = getAnalyzerCheckersAndPackages(
       Context, Context.canEnableAnalyzerAlphaCheckers());
   if (!AnalyzerOptions->CheckersAndPackages.empty()) {
     setStaticAnalyzerCheckerOpts(Context.getOptions(), *AnalyzerOptions);
     AnalyzerOptions->AnalysisStoreOpt = RegionStoreModel;
     AnalyzerOptions->AnalysisDiagOpt = PD_NONE;
     AnalyzerOptions->AnalyzeNestedBlocks = true;
     AnalyzerOptions->eagerlyAssumeBinOpBifurcation = true;
     std::unique_ptr<ento::AnalysisASTConsumer> AnalysisConsumer =
         ento::CreateAnalysisConsumer(Compiler);
     AnalysisConsumer->AddDiagnosticConsumer(
         new AnalyzerDiagnosticConsumer(Context));
     Consumers.push_back(std::move(AnalysisConsumer));
   }
 #endif // CLANG_TIDY_ENABLE_STATIC_ANALYZER
   return std::make_unique<ClangTidyASTConsumer>(
       std::move(Consumers), std::move(Profiling), std::move(Finder),
-      std::move(Checks));
+      FinderASTConsumerPtr, SharedDeclFilter, std::move(Checks));
 }
 
 std::vector<std::string> ClangTidyASTConsumerFactory::getCheckNames() {
   std::vector<std::string> CheckNames;
   for (const auto &CheckFactory : *CheckFactories) {
     if (Context.isCheckEnabled(CheckFactory.getKey()))
       CheckNames.emplace_back(CheckFactory.getKey());
   }

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

case clang::DiagnosticsEngine::ak_addrspace:
       break;
     }
   }
 }
 
 void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
                                                const SourceManager &Sources) {
   // Invalid location may mean a diagnostic in a command line, don't skip these.
-  if (!Location.isValid()) {
+  if (!Location.isValid() || *Context.getOptions().ShowAllWarnings) {
     LastErrorRelatesToUserCode = true;
     LastErrorPassesLineFilter = true;
     return;
   }
 
   if (!*Context.getOptions().SystemHeaders &&
       Sources.isInSystemHeader(Location))
     return;

clang-tools-extra/clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
 
   /// WarningsAsErrors filter.
   llvm::Optional<std::string> WarningsAsErrors;
 
   /// Output warnings from headers matching this filter. Warnings from
   /// main files will always be displayed.
   llvm::Optional<std::string> HeaderFilterRegex;
 
+  /// Show all warnings, including warnings from all header files.
+  llvm::Optional<bool> ShowAllWarnings;
+
+  /// Do not check included files, except files matching the --header-filter
+  /// and system files when --system-headers is used.
+  llvm::Optional<bool> SkipHeaders;
+
   /// Output warnings from system headers matching \c HeaderFilterRegex.
   llvm::Optional<bool> SystemHeaders;
 
   /// Format code around applied fixes with clang-format using this
   /// style.
   ///
   /// Can be one of:
   ///   * 'none' - don't format code around applied fixes;

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

static void mapping(IO &IO, ClangTidyOptions &Options) {
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // legacy compatibility
     IO.mapOptional("FormatStyle", Options.FormatStyle);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
     IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
     IO.mapOptional("UseColor", Options.UseColor);
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
 
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
   Options.WarningsAsErrors = "";
   Options.HeaderFilterRegex = "";
+  Options.ShowAllWarnings = false;
+  Options.SkipHeaders = false;
   Options.SystemHeaders = false;
   Options.FormatStyle = "none";
   Options.User = llvm::None;
   for (const ClangTidyModuleRegistry::entry &Module :
        ClangTidyModuleRegistry::entries())
     Options.mergeWith(Module.instantiate()->getModuleOptions(), 0);
   return Options;
 }
if (Src)
     Dest = Src;
 }
 
 ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
                                               unsigned Order) {
   mergeCommaSeparatedLists(Checks, Other.Checks);
   mergeCommaSeparatedLists(WarningsAsErrors, Other.WarningsAsErrors);
   overrideValue(HeaderFilterRegex, Other.HeaderFilterRegex);
+  overrideValue(ShowAllWarnings, Other.ShowAllWarnings);
+  overrideValue(SkipHeaders, Other.SkipHeaders);
   overrideValue(SystemHeaders, Other.SystemHeaders);
   overrideValue(FormatStyle, Other.FormatStyle);
   overrideValue(User, Other.User);
   overrideValue(UseColor, Other.UseColor);
   mergeVectors(ExtraArgs, Other.ExtraArgs);
   mergeVectors(ExtraArgsBefore, Other.ExtraArgsBefore);
 
   for (const auto &KeyValue : Other.CheckOptions) {

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

 always displayed.
 Can be used together with -line-filter.
 This option overrides the 'HeaderFilterRegex'
 option in .clang-tidy file, if any.
 )"),
                                          cl::init(""),
                                          cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> ShowAllWarnings("show-all-warnings",
+                                     cl::desc("Display all warning messages."),
+                                     cl::init(false), cl::Hidden,
+                                     cl::cat(ClangTidyCategory));
+
+static cl::opt<bool> SkipHeaders("skip-headers", cl::desc(R"(
+Do not check included header files, but
+files matching the --header-filter pattern are still checked.
+System headers are still checked if --system-headers is true.
+)"),
+                                 cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<bool>
     SystemHeaders("system-headers",
                   cl::desc("Display the errors from system headers."),
                   cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> LineFilter("line-filter", cl::desc(R"(
 List of files with line ranges to filter the
 warnings. Can be used together with
 -header-filter. The format of the list is a
 JSON array of objects:
   [
     {"name":"file1.cpp","lines":[[1,3],[5,7]]},
     {"name":"file2.h"}
if (std::error_code Err = parseLineFilter(LineFilter, GlobalOptions)) {
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return nullptr;
   }
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
+  DefaultOptions.ShowAllWarnings = ShowAllWarnings;
+  DefaultOptions.SkipHeaders = SkipHeaders;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.FormatStyle = FormatStyle;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
     DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
     OverrideOptions.Checks = Checks;
   if (WarningsAsErrors.getNumOccurrences() > 0)
     OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
+  if (ShowAllWarnings.getNumOccurrences() > 0)
+    OverrideOptions.ShowAllWarnings = ShowAllWarnings;
+  if (SkipHeaders.getNumOccurrences() > 0)
+    OverrideOptions.SkipHeaders = SkipHeaders;
   if (SystemHeaders.getNumOccurrences() > 0)
     OverrideOptions.SystemHeaders = SystemHeaders;
   if (FormatStyle.getNumOccurrences() > 0)
     OverrideOptions.FormatStyle = FormatStyle;
   if (UseColor.getNumOccurrences() > 0)
     OverrideOptions.UseColor = UseColor;
 
   auto LoadConfig =

clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header1.h

+// no header guard
+
+#include "my_header2.h"

clang-tools-extra/test/clang-tidy/checkers/Inputs/skip-headers/my_header2.h

+// bad header guard
+#ifndef SOME_MACRO
+
+int abc = 123; // bad definition in .h file
+
+#endif

clang-tools-extra/test/clang-tidy/checkers/bugprone-forward-declaration-namespace.cpp

 // RUN: %check_clang_tidy %s bugprone-forward-declaration-namespace %t
+// RUN: %check_clang_tidy %s bugprone-forward-declaration-namespace %t -- --skip-headers
 
 namespace {
 // This is a declaration in a wrong namespace.
 class T_A;
 // CHECK-NOTES: :[[@LINE-1]]:7: warning: declaration 'T_A' is never referenced, but a declaration with the same name found in another namespace 'na' [bugprone-forward-declaration-namespace]
 // CHECK-NOTES: note: a declaration of 'T_A' is found here
 // CHECK-NOTES: :[[@LINE-3]]:7: warning: no definition found for 'T_A', but a definition with the same name 'T_A' found in another namespace '(global)' [bugprone-forward-declaration-namespace]
 // CHECK-NOTES: note: a definition of 'T_A' is found here

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp

 // RUN: %check_clang_tidy %s bugprone-reserved-identifier %t -- -- \
 // RUN:   -I%S/Inputs/bugprone-reserved-identifier \
 // RUN:   -isystem %S/Inputs/bugprone-reserved-identifier/system
+// RUN: %check_clang_tidy %s bugprone-reserved-identifier %t -- --skip-headers -- \
+// RUN:   -I%S/Inputs/bugprone-reserved-identifier \
+// RUN:   -isystem %S/Inputs/bugprone-reserved-identifier/system
 
 // no warnings expected without -header-filter=
 #include "user-header.h"
 #include <system-header.h>
 
 #define _MACRO(m) int m = 0
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_MACRO', which is a reserved identifier [bugprone-reserved-identifier]
 // CHECK-FIXES: {{^}}#define MACRO(m) int m = 0{{$}}

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

 // RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- --skip-headers -- -isystem %S/Inputs/Headers -fmodules
 
 // clang-format off
 
 // CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
 // CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
 // CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
 // CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
 #include "a.cpp"

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-interfaces-global-init.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-interfaces-global-init %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-interfaces-global-init %t -- --skip-headers
 
 constexpr int makesInt() { return 3; }
 constexpr int takesInt(int i) { return i + 1; }
 constexpr int takesIntPtr(int *i) { return *i; }
 
 extern int ExternGlobal;
 static int GlobalScopeBadInit1 = ExternGlobal;
 // CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'

clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp

-// RUN: clang-tidy %s -checks='-*,google-build-namespaces,google-build-using-namespace' -header-filter='.*' -- | FileCheck %s -implicit-check-not="{{warning|error}}:"
+// RUN: clang-tidy %s -checks='-*,google-build-namespaces,google-build-using-namespace' -header-filter='.*' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes CHECK,CHECK1 -implicit-check-not="{{warning|error}}:"
+
+// RUN: clang-tidy %s -checks='-*,google-build-namespaces,google-build-using-namespace' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes CHECK,CHECK0 -implicit-check-not="{{warning|error}}:"
+//
+// RUN: clang-tidy %s --skip-headers -checks='-*,google-build-namespaces,google-build-using-namespace' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes CHECK,CHECK2 -implicit-check-not="{{warning|error}}:"
+//
+// -header-filter overrides --skip-header
+// RUN: clang-tidy %s --skip-headers -header-filter='.*' -checks='-*,google-build-namespaces,google-build-using-namespace' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes CHECK,CHECK1 -implicit-check-not="{{warning|error}}:"
+//
+// --show-all-warnings is like -header-filter=.* + -system-headers
+// RUN: clang-tidy %s --show-all-warnings -checks='-*,google-build-namespaces,google-build-using-namespace' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes CHECK,CHECK1 -implicit-check-not="{{warning|error}}:"
+
+// --skip-header skips 1 warning in the header file.
+// CHECK0: 7 warnings generated
+// CHECK1: 7 warnings generated
+// CHECK2: 6 warnings generated
+
 #include "Inputs/google-namespaces.h"
-// CHECK: warning: do not use unnamed namespaces in header files [google-build-namespaces]
+// with -header-filter, warning in .h file is shown
+// CHECK1: warning: do not use unnamed namespaces in header files [google-build-namespaces]
+// without -header-filter, warning in .h files are not shown
+// CHECK0-NOT: warning: do not use unnamed namespaces in header files [google-build-namespaces]
+// with --skip-header, no warning in .h file is detected at all
+// CHECK2-NOT: warning: do not use unnamed namespaces in header files [google-build-namespaces]
 
 using namespace spaaaace;
 // CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
 
 using spaaaace::core; // no-warning
 
 namespace std {
 inline namespace literals {

 
 using namespace foo::literals::bar_literals;
 // CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
 
 namespace foo_literals {}
 
 using namespace foo_literals;
 // CHECK: :[[@LINE-1]]:1: warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]
+
+// If -header-filter= is not used and there is some warning in .h file,
+// give a reminder to use -header-filter.
+// CHECK0: Use -header-filter={{.*}} to display errors{{.*}}
+//
+// If -header-filter= is used, no summary of this message.
+// CHECK1-NOT: Use -header-filter={{.*}} to display errors{{.*}}
+//
+// With --skip-header, no warning in .h file is detected or hidden,
+// no need to give a reminder to use -header-filter.
+// CHECK2-NOT: Use -header-filter={{.*}} to display errors{{.*}}

clang-tools-extra/test/clang-tidy/checkers/google-objc-function-naming.m

 // RUN: %check_clang_tidy %s google-objc-function-naming %t -- -- -isystem %S/Inputs/Headers
+// RUN: %check_clang_tidy %s google-objc-function-naming %t -- --skip-headers -- -isystem %S/Inputs/Headers
 
 #include <stdio.h>
 
 static void TestImplicitFunctionDeclaration(int a) {
   // Call a builtin function so that the compiler generates an implicit
   // function declaration.
   printf("%d", a);
 }

clang-tools-extra/test/clang-tidy/checkers/llvm-include-order.cpp

 // RUN: %check_clang_tidy %s llvm-include-order %t -- -- -isystem %S/Inputs/Headers
+// RUN: %check_clang_tidy %s llvm-include-order %t -- --skip-headers -- -isystem %S/Inputs/Headers
 
 // CHECK-MESSAGES: [[@LINE+2]]:1: warning: #includes are not sorted properly
 #include "j.h"
 #include "gtest/foo.h"
 #include "gmock/foo.h"
 #include "i.h"
 #include <s.h>
 #include "llvm/a.h"

clang-tools-extra/test/clang-tidy/checkers/llvm-prefer-register-over-unsigned.cpp

 // RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t -- --skip-headers
 
 namespace llvm {
 class Register {
 public:
   operator unsigned();
 
   unsigned Reg;
 };

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

 // RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t -- --skip-headers
 
 #define MACRO_A "defining macros outside namespace is valid"
 
 class ClassB;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace
 struct StructC {};
 // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be declared within the '__llvm_libc' namespace
 char *VarD = MACRO_A;

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp

 // RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
 // RUN:   -- -- -isystem %S/Inputs/llvmlibc/system \
 // RUN:   -resource-dir %S/Inputs/llvmlibc/resource
+//
+// RUN: %check_clang_tidy %s llvmlibc-restrict-system-libc-headers %t \
+// RUN:   -- --skip-headers -- -isystem %S/Inputs/llvmlibc/system \
+// RUN:   -resource-dir %S/Inputs/llvmlibc/resource
 
 #include <stdio.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdio.h not allowed
 #include <stdlib.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdlib.h not allowed
 #include "string.h"
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include string.h not allowed
 #include "stdatomic.h"
 #include <stddef.h>
 // Compiler provided headers should not throw warnings.

clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp

 // RUN: %check_clang_tidy %s misc-no-recursion %t
+// RUN: %check_clang_tidy %s misc-no-recursion %t -- --skip-headers
 
 // We don't have the definition of this function,
 // so we can't tell anything about it..
 void external();
 
 // This function is obviously not recursive.
 void no_recursion() {
 }

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx03.cpp

 // RUN: %check_clang_tidy -std=c++98 %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers
+// RUN: %check_clang_tidy -std=c++98 %s modernize-deprecated-headers %t -- --skip-headers -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers
 
 #include <assert.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
 // CHECK-FIXES: {{^}}#include <cassert>{{$}}
 #include <complex.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'complex.h'; consider using 'complex' instead
 // CHECK-FIXES: {{^}}#include <complex>{{$}}
 #include <ctype.h>

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-cxx11.cpp

 // RUN: %check_clang_tidy -std=c++11-or-later %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers
+// RUN: %check_clang_tidy -std=c++11-or-later %s modernize-deprecated-headers %t -- --skip-headers -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers
 
 #include <assert.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
 // CHECK-FIXES: {{^}}#include <cassert>{{$}}
 #include <complex.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'complex.h'; consider using 'complex' instead
 // CHECK-FIXES: {{^}}#include <complex>{{$}}
 #include <ctype.h>

clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp

 // RUN: cp %S/Inputs/modernize-pass-by-value/header.h %T/pass-by-value-header.h
-// RUN: clang-tidy %s -checks='-*,modernize-pass-by-value' -header-filter='.*' -fix -- -std=c++11 -I %T | FileCheck %s -check-prefix=CHECK-MESSAGES -implicit-check-not="{{warning|error}}:"
+// RUN: clang-tidy %s -checks='-*,modernize-pass-by-value' -header-filter='.*' -fix -- -std=c++11 -I %T \
+// RUN: | FileCheck %s -check-prefix=CHECK-MESSAGES -implicit-check-not="{{warning|error}}:"
 // RUN: FileCheck -input-file=%T/pass-by-value-header.h %s -check-prefix=CHECK-FIXES
+//
+// RUN: cp %S/Inputs/modernize-pass-by-value/header.h %T/pass-by-value-header.h
+// RUN: clang-tidy %s -checks='*' -- -std=c++11 -I %T \
+// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-NOHEADER -allow-empty
+//
+// RUN: cp %S/Inputs/modernize-pass-by-value/header.h %T/pass-by-value-header.h
+// RUN: clang-tidy %s -checks='-*,modernize-pass-by-value' --skip-headers -fix -- -std=c++11 -I %T \
+// RUN: | FileCheck %s -check-prefix=CHECK-SKIP -allow-empty -implicit-check-not="{{warning|error}}:"
+// RUN: FileCheck -input-file=%T/pass-by-value-header.h %s -check-prefix=CHECK-SKIP-FIXES
+//
+// RUN: cp %S/Inputs/modernize-pass-by-value/header.h %T/pass-by-value-header.h
+// RUN: clang-tidy %s -checks='*' --skip-headers -- -std=c++11 -I %T \
+// RUN: | FileCheck %s -check-prefix=CHECK-SKIP -allow-empty -implicit-check-not="{{warning|error}}:"
+//
 // FIXME: Make the test work in all language modes.
 
 #include "pass-by-value-header.h"
-// CHECK-MESSAGES: :8:5: warning: pass by value and use std::move [modernize-pass-by-value]
+// header file warnings are not shown, with --skip-headers, or without -header-filter
+// CHECK-MESSAGES:     :8:5: warning: pass by value and use std::move [modernize-pass-by-value]
+// CHECK-NOHEADER-NOT: :8:5: warning: pass by value and use std::move [modernize-pass-by-value]
+// CHECK-SKIP-NOT:     :8:5: warning: pass by value and use std::move [modernize-pass-by-value]
+//
 // CHECK-FIXES: #include <utility>
 // CHECK-FIXES: A(ThreadId tid) : threadid(std::move(tid)) {}
+// CHECK-SKIP-FIXES-NOT: #include <utility>
+//
+// header files are checked even without -header-filtler
+// CHECK-NOHEADER: Suppressed {{.*}} warnings
+// CHECK-NOHEADER: Use -header-filter={{.*}} to display errors{{.*}}
+//
+// header files are not checked with --skip-headers
+// CHECK-SKIP-NOT: Suppressed {{.*}} warnings
+// CHECK-SKIP-NOT: Use -header-filter={{.*}} to display errors{{.*}}

clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp

 // RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
 // RUN:     -- -config="{CheckOptions: [{key: portability-restrict-system-includes.Includes, value: '*,-stddef.h'}]}" \
 // RUN:     -- -isystem %S/Inputs/portability-restrict-system-includes/system
+//
+// RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
+// RUN:     -- -config="{CheckOptions: [{key: portability-restrict-system-includes.Includes, value: '*,-stddef.h'}]}" --skip-headers \
+// RUN:     -- -isystem %S/Inputs/portability-restrict-system-includes/system
 
 // Test block-list functionality: allow all but stddef.h.
 
 #include <stddef.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stddef.h not allowed
 #include <stdint.h>
 #include <float.h>

clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp

 // RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
 // RUN:     -- -config="{CheckOptions: [{key: portability-restrict-system-includes.Includes, value: '-*,stddef.h'}]}" \
 // RUN:     -- -isystem %S/Inputs/portability-restrict-system-includes/system
+//
+// RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
+// RUN:     -- -config="{CheckOptions: [{key: portability-restrict-system-includes.Includes, value: '-*,stddef.h'}]}" --skip-headers \
+// RUN:     -- -isystem %S/Inputs/portability-restrict-system-includes/system
 
 // Test allow-list functionality: disallow all but stddef.h.
 
 #include <stddef.h>
 #include <stdint.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdint.h not allowed
 #include <float.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include float.h not allowed

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-assignment.cpp

-// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: true}]}" --
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- \
+// RUN: -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: true}]}" --
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- --skip-headers \
+// RUN: -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalAssignment", value: true}]}" --
 
 void chained_conditional_compound_assignment(int i) {
   bool b;
   if (i < 0) {
     b = true;
   } else if (i < 10) {
     b = false;
   } else if (i > 20) {

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-chained-conditional-return.cpp

-// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: true}]}" --
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- \
+// RUN: -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: true}]}" --
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- --skip-headers \
+// RUN: -config="{CheckOptions: [{key: "readability-simplify-boolean-expr.ChainedConditionalReturn", value: true}]}" --
 
 bool chained_conditional_compound_return(int i) {
   if (i < 0) {
     return true;
   } else if (i < 10) {
     return false;
   } else if (i > 20) {
     return true;

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr-members.cpp

 // RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- --skip-headers
 
 class A {
 public:
   int m;
 };
 
 struct S {
   S() : m_ar(s_a) {}

clang-tools-extra/test/clang-tidy/checkers/readability-simplify-bool-expr.cpp

 // RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -- --skip-headers
 
 bool a1 = false;
 
 //=-=-=-=-=-=-= operator ==
 bool aa = false == a1;
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
 // CHECK-FIXES: {{^bool aa = !a1;$}}
 bool ab = true == a1;

clang-tools-extra/test/clang-tidy/checkers/skip-headers.cpp

+// Test --skip-headers, --show-all-warnings, and --header-filter.
+// TODO: when skip-headers implementation is complete, add back
+//      -implicit-check-not="{{warning|error}}:"
+// and use no_hint instead of hint
+//
+// Default shows no warning in .h files, and give HINT to use --header-filter
+// RUN: clang-tidy %s -checks='*' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,MAIN,HINT
+// later:    -implicit-check-not="{{warning|error}}:"
+//
+// --skip-headers skips included files; finds only warnings in the main file.
+// RUN: clang-tidy %s -checks='*' --skip-headers -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,MAIN,HINT
+// later:    no_hint -implicit-check-not="{{warning|error}}:"
+//
+// --show-all-warnings reports all warnings, even without --header-filters
+// RUN: clang-tidy %s -checks='*' --show-all-warnings -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,WARN_BOTH,MAIN,NO_HINT
+//
+// --header-filter='.*' is like --show-all-warnings
+// RUN: clang-tidy %s -checks='*' --header-filter='.*' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,WARN_BOTH,MAIN,NO_HINT
+//
+// --header-filter='header1.h' shows only warnings in header1.h
+// RUN: clang-tidy %s -checks='*' --header-filter='header1.h' -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,WARN1,MAIN,HINT
+// later:    -implicit-check-not="{{warning|error}}:"
+//
+// The main purpose of --show-all-warnings is to debug --skip-headers.
+// When used together, no warnings should be reported from header files.
+// RUN: clang-tidy %s -checks='*' --skip-headers --show-all-warnings -- \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,MAIN,NO_HINT
+// later:  -implicit-check-not="{{warning|error}}:"
+//
+// use --skip-headers and --header-filter='header2.h'
+// to skip header1.h but not header2.h
+// RUN: clang-tidy %s -checks='*' --skip-headers --header-filter='header2.h' \
+// RUN: 2>&1 | FileCheck %s -check-prefixes WARN,WARN2,MAIN,HINT
+// later:  no_hint
+
+// WARN: {{[0-9]+}} warnings generated.
+#include "Inputs/skip-headers/my_header1.h"
+// WARN1: my_header1.h:1:1: warning: header is missing header guard
+// WARN1-NOT: my_header2.h
+// WARN2: my_header2.h:1:1: warning: header is missing header guard
+// WARN2-NOT: my_header1.h
+// WARN_BOTH: my_header1.h:1:1: warning: header is missing header guard
+// WARN_BOTH: my_header2.h:1:1: warning: header is missing header guard
+
+int xyz = 135;
+// MAIN-COUNT-4: skip-headers.cpp:{{[0-9]+}}:{{[0-9]+}}: warning:
+
+// HINT: Use -header-filter={{.*}} to display errors{{.*}}
+// NO_HINT-NOT: Use -header-filter=

clang/include/clang/Frontend/MultiplexConsumer.h

public:
   ASTDeserializationListener *GetASTDeserializationListener() override;
   void PrintStats() override;
   bool shouldSkipFunctionBody(Decl *D) override;
 
   // SemaConsumer
   void InitializeSema(Sema &S) override;
   void ForgetSema() override;
 
-private:
+protected:
   std::vector<std::unique_ptr<ASTConsumer>> Consumers; // Owns these.
+private:
   std::unique_ptr<MultiplexASTMutationListener> MutationListener;
   std::unique_ptr<MultiplexASTDeserializationListener> DeserializationListener;
 };
 
 }  // end namespace clang
 
 #endif