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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks much closer.

Customizing the MatchFinder::matchAST() to set the scope is a neat try but I don't think the design really fits (and we have to abuse inheritance to make it work).
Ultimately you need to customize both near `HandleTranslationUnit` and near `HandleTopLevelDecl`, and especially the latter really is an ASTConsumer concern.
So implementing an ASTConsumer in some form seems necessary.

> I reproduced the ClangdTests::TargetDeclTest.Concept failure locally without this change,

I'll have a look, can't reproduce it locally. Have you seen it on any buildbots?



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:434
+    return false;
+  auto FID = Sources.getDecomposedExpansionLoc(Location).first;
+  // Quick check against last checked results.
----------------
chh wrote:
> chh wrote:
> > sammccall wrote:
> > > 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?
> > This is to match the ClangTidyDiagnosticConsumer::checkFilters.
> > Do you think some other get*Location method should be used in both places?
> > 
> I would rather explore various cache performance improvements in follow up CLs.
> In my performance tests, current simple check of LastAcceptedFileID and LastSkippedFileID already has very high hit rate due to sequential check of Decls in the included header files. More caching improvements should produce relatively smaller difference, which will be shown/visible easier in their own small change CLs.
> 
> Do you think some other get*Location method should be used in both places?

It depends on what you want to achieve. Matching the check-filters behavior elsewhere makes sense.
(It does raise the question though why the code can't be shared, even if the line filter doesn't apply per se)


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:473
+    MatchFinder::matchAST(Context);
+    Context.setTraversalScope(SavedScope);
+  } else {
----------------
Is restoring the traversal scope strictly needed by the static analyzer? I would expect not, but I might be wrong.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:144
   MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
-  ~MatchFinder();
+  virtual ~MatchFinder();
+
----------------
Matchfinder specifically says "not intended to be subclassed".


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:147
+  /// Inside a MatchASTConsumer, handles all top level Decl.
+  virtual bool HandleTopLevelDecl(DeclGroupRef DG);
 
----------------
This function doesn't make sense at this layer - calling it does nothing and the class is intended to be used directly, not to be subclassed. And MatchFinder itself doesn't "consume" top-level decls - that's an ASTConsumer concern.

I understand you use it here as a hook to listen for decls seen by the MatchFinder::newASTConsumer().
Better would be to pass a HandleTopLevelDecl callback to newASTConsumer() instead.
But given that MatchFinderASTConsumer is completely trivial and can be built on the public MatchFinder API, better still would just be to reimplement it with the unusual functionality ClangTidy needs.


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

https://reviews.llvm.org/D98710