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