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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Back from vacation!



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:420
+  if (!File)
+    return false; // do not skip non-file locations
+  StringRef FileName(File->getName());
----------------
this is probably worth some explanation


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:431
+bool DeclFilter::skipLocation(SourceLocation Location) {
+  // do not skip non-file locations
+  if (!Location.isValid())
----------------
this comment didn't match the next line.
Did you intend to check Location.isFile() instead?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:434
+    return false;
+  auto FID = Sources.getDecomposedExpansionLoc(Location).first;
+  // Quick check against last checked results.
----------------
maybe add a comment explaning why the expansion loc is chosen?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:434
+    return false;
+  auto FID = Sources.getDecomposedExpansionLoc(Location).first;
+  // Quick check against last checked results.
----------------
sammccall wrote:
> maybe add a comment explaning why the expansion loc is chosen?
note that this means we're going to traverse up the macro expansion edges even if we're hitting the cache. You may want to cache under the original file ID instead?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:436
+  // Quick check against last checked results.
+  if (FID == LastSkippedFileID)
+    return true;
----------------
caching all results in a DenseMap<FileID, bool> seems likely to perform better, no?


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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98710/new/

https://reviews.llvm.org/D98710