chh added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:31 + +struct DeclFilter : public OptionsDeclFilter { + DeclFilter(ClangTidyContext &Ctx, SourceManager &SM) ---------------- sammccall wrote: > 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) > > Thanks. Moved this class to ClangTidy.cpp. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:148 + /// Check if a Decl should be skipped. + std::shared_ptr<DeclFilter> Filter; }; ---------------- sammccall wrote: > 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. > 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. 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