[PATCH] D98710: [clang-tidy] Add --skip-headers, part 1 (use setTraversalScope)

[Haojian Wu via Phabricator via cfe-commits]

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