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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
