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
