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

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:31
+
+struct DeclFilter : public OptionsDeclFilter {
+  DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
----------------
sammccall wrote:
> The layering in clang-tidy is... not particularly great or clear, but there is a distinction between driver-y type code (the clang-tidy application) vs the checks themselves.
> 
> As I understand our plan, this filter is "just" a way to configure the traversal scope before running, i.e. what information the AST will expose to the checks.
> That is, this is a driver concern and should not be visible to the checks - probably should go in `ClangTidy.h` or even just `ClangTidy.cpp` as part of the `ClangTidyASTConsumer`. My litmus test here is that we do have other drivers (at least clangd that I know of), and they shouldn't be seeing/reusing this mechanism, and so neither should checks.
> 
> (Similarly if we do something with PP in the future, hopefully this can *also* be transparent to checks)
> 
> 
Thanks. Moved this class to ClangTidy.cpp.



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




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

https://reviews.llvm.org/D98710