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

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

chh marked an inline comment as done and an inline comment as not done.
chh added a comment.

Last build tests were green. Let's see if the new updated diff still passes all the tests.

Looks like all choices have some risk/overhead in future maintenance and we need to pick one with the least cost.
I think cloning classes is most expensive and risky to keep future compatibility.
Adding a new subclass or optional members into existing class are more conventional solutions.



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:473
+    MatchFinder::matchAST(Context);
+    Context.setTraversalScope(SavedScope);
+  } else {
----------------
sammccall wrote:
> Is restoring the traversal scope strictly needed by the static analyzer? I would expect not, but I might be wrong.
I think it is safer to assume low or acceptable overhead in get/setTraversalScope and keep the impact only to the MatchFinder consumer, rather than depending on static analyzer working now or in the future with the changed AST.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:144
   MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
-  ~MatchFinder();
+  virtual ~MatchFinder();
+
----------------
sammccall wrote:
> Matchfinder specifically says "not intended to be subclassed".
Do you know the reason it cannot have subclass?
All our current implementation choices need to change its matchAST behavior, either by overriding it, or changing the AST before it, or skip Decl in the recursive walk of AST.
For any reason not to have subclass, changing AST outside matchAST seems riskier than changing inside matchAST in a child class.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:147
+  /// Inside a MatchASTConsumer, handles all top level Decl.
+  virtual bool HandleTopLevelDecl(DeclGroupRef DG);
 
----------------
sammccall wrote:
> 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.
Please review the updated diff. Now HandleTopLevelDecl is all in clang-tidy.
For both the MultiplexConsumer and MatchFinder, I think it is more feasible to resolve all subclass issues now and maintain the inheritance relation than making clone implementations and trying to maintain the same behavior in the future.



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

https://reviews.llvm.org/D98710