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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits