sammccall 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:
> > hokein wrote:
> > > 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;
> > > };
> > > ```
> > MultiplexConsumer is just used as a helper to glue together two concrete
> > ASTConsumers, not arbitrary consumers, so we can reason about whether it's
> > safe by reading the code.
> >
> > The static analyzer's consumer appears to already track top-level decls
> > rather than traversing from the root, exactly to avoid traversing stuff
> > from the preamble. So I would expect setTraversalContext to work fine there.
> >
> > The approach Haojian suggests of avoiding MultiplexConsumer, and making the
> > tight coupling to the particular consumers explicit, also seems fine.
> >
> > > If that's correct, it seems to me that changing those two handlers in
> > > MatchFinder is right for clang-tidy
> >
> > This might be the most convenient thing for clang-tidy, but I don't think
> > it's the right thing for MatchFinder, which is also important.
> I looked into Hokein's suggestion. Although in general correctly used
> inheritance is better, there are special cases that copying functionality
> from another class could be better. I wondered if it could work better by
> changing ClangTidyASTConsumer to behave like MultiplexConsumer without
> inheritance.
>
> After looking into MultiplexConsumer's many methods, what are used and could
> be used in the future by MatchFinder's MatchASTConsumer and static analyzer's
> AnalysisConsumer, the dependency on MultiplexConsumer to pass through all
> Consumer methods is obvious. So, ClangTidyASTConsumer really needs to behave
> like MultiplexConsumer, duplicating all existing and future methods, to keep
> MatchFinder and AnalysisConsumer working. This looks like a real inheritance
> use case and difficult to make it better without inheritance.
>
> If MatchFinder users really don't want an optional filter for
> HandleTranslationUnit and HandleTopLevelDecl, I think an alternative is to
> define a child class maybe called TidyMatchFinder with the extra features
> needed now only by clang-tidy. But, there is also advantage in adding these
> into MatchFinder now for future non-clang-tidy users. That will be similar to
> the current optional MatchFinder's CheckProfiling and the
> set/getTraversalScope. Those features/interfaces started with one user/tool
> and now we see more users/tools. If they were defined in a child class
> specific to clangd or some profiler, we will now need to rename or move those
> features to a common base class to share.
>
> Please feel free to suggest any alternative, or let me know if you are okay
> with current optional filter in MatchFinder, or if you insist on defining a
> new child class like TidyMatchFinder.
> Thanks.
>
TL;DR is I think this is a clang-tidy feature that should not be visible in the
matcher layer in any way.
If it's going to touch ASTMatcher I think we should have a strong design reason
or a very strong implementation reason. That it's kind of awkward to code and
might one day be useful isn't sufficient.
There are four layers here: AST nodes themselves, RecursiveASTVisitor (which
defines a tree via parent/child relationships), the matcher framework, and
finally clang-tidy.
The mechanics of filtering using a list are solved in RecursiveASTVisitor.
There seem to be two issues here against solving the rest in the clang-tidy
layer.
---
First is conceptual: is having filter APIs in the matcher framework good? I
don't think so, because:
- it's duplicative: it mirrors the setTraversalScope functionality and it's
unclear when to use which. Having two ways to accomplish a rare task seems
excessive.
- it conflicts with the design of this layer: matchers are all about
non-destructive reading, but setting traversal scope has side effects. So it's
only really usable with the ASTConsumer, which is a corner of the interface.
- it's not necessary: it's always possible to get at the underlying ASTContext
wherever a MatchFinder will be used.
You suggest this might be good for "future non-clang-tidy users" but I don't
think we should add it speculatively for this purpose. It seems at least as
likely that such users will never materialize, or that the solution that is
right for clang-tidy is not right for them.
---
Second is practical. That ClangTidyASTConsumerFactory::CreateASTConsumer needs
to dispatch both to MatchFinder and the static analyzer's (non-trivial)
ASTConsumer. We have to add a call to setTraversalScope somewhere, and none of
the approaches are perfectly natural.
However there are several that are OK:
- subclass MultiplexingASTConsumer to inject the call in HandleTranslationUnit
- subclass MultiplexingASTConsumer and inject the call and also the
matcher-based-checks logic in HandleTranslationUnit (so MAC delegates only to
the static analyzer)
- write an ASTConsumer without MultiplexingASTConsumer that manually forwards
only the actually needed methods to the static analyzer consumer
- write an unrelated ASTConsumer that forwards every method to the static
analyzer consumer
- refactor static analyzer to expose its consumer class, and subclass that
(yuck!)
These do require making *some* assumptions about how these consumers work. But
that's OK - there are two of them, not thousands, and these interfaces change
rarely. We don't need to architect defensively against any possible future
change, and that's not culturally how LLVM operates either. I'd lean towards
e.g. the first option, which requires fewer assumptions.
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
