[PATCH] D98710: [clang-tidy] Add --skip-headers, part 1 (use setTraversalScope)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:31
+
+struct DeclFilter : public OptionsDeclFilter {
+  DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
----------------
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)




================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:148
+    /// Check if a Decl should be skipped.
+    std::shared_ptr<DeclFilter> Filter;
   };
----------------
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.



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

https://reviews.llvm.org/D98710