chh added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:310
+struct DeclFilter {
+ DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
+ : Context(Ctx), Sources(SM) {
----------------
sammccall wrote:
> Ctx is ultimately unused, just take the regex instead?
Context is used in skipFileID.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:514
+ for (auto &Consumer : Consumers) {
+ if (hasFilter() && Consumer.get() == FinderASTConsumerPtr) {
+ // Modify AST before calling the Finder's ASTConsumer.
----------------
sammccall wrote:
> This adds significant conceptual complexity and possible runtime costs to
> guard against the possibility that the static analyzer would not be
> compatible with simply setting the traversal scope.
>
> Please do not do this. It may appear that you're cheaply eliminating a class
> of bugs, but complexity is not cheap, and in the long run can also introduce
> bugs. Instead investigate whether this is a real problem and why.
>
> If it's not then `Context.setTraversalScope()` +
> `MultiplexingASTConsumer::HandleTranslationUnit` is enough here, and
> `FinderASTConsumerPtr` can go away.
For this first step implementation of skip-headers, could we limit the risk and
impact to only AST MatchFinder based checks? If it works fine, we can add more
performance improvements, handle PPCallback-based checks, and static analyzer
checks. We can turn skip-headers to default or revert any follow up step.
At this time, we only know that PPCallback-based checks can save some more time
with skip-headers, but do not know if any static analyzer check can benefit
from skip-headers easily. In other words, we are sure to deliver big saving of
runtime for clang-tidy users that do not enable static analyzer checks, but not
sure about static analyzer check users.
The overhead and complexity of set/getTraversalScope and checking filters will
be on the users of skip-header, but not on the static analyzer checkers.
If we apply the same change to AST for all consumers, the saving of code
complexity is smaller than the risk of impact to static analyzer checks, and
the saving of runtime is also much smaller than the already saved time in
MatchFinder.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:473
+ MatchFinder::matchAST(Context);
+ Context.setTraversalScope(SavedScope);
+ } else {
----------------
sammccall wrote:
> chh wrote:
> > 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.
> >
> > I think it is safer to assume low or acceptable overhead in
> > get/setTraversalScope
> We have reasons to believe this is not cheap. The first usage of getParent()
> after changing traversal scope incurs a full AST traversal.
>
> > rather than depending on static analyzer working now or in the future with
> > the changed AST.
>
> There are a fairly small number of reasons that setTraversalScope could break
> static analyzer at this point, and I think they're all going to cause the
> static analyzer to be slow when used with PCHes. However the static
> analyzer's implementation and comments say that it goes to effort to be fast
> with PCHes. So I do believe this is a more stable assumption, and in the long
> run we may be able to simplify its implementation.
Okay, won't assume low overhead in set/getTraversalScope.
So far their impact is limited to skip-headers, which in this step is limited
to only MatchFinder. Please see also my reply in the other comment.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:99
IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
IO.mapOptional("UseColor", Options.UseColor);
}
----------------
sammccall wrote:
> Maybe an explicit comment that ShowAllHeaders, SkipHeaders are
> runtime/optimization options that are configured at the command-line only,
> and therefore not mapped here
Why SystemHeaders is not mapped?
I agree that ShowAllWarnings is not important here,
but SkipHeaders seems as important to be included here as HeaderFilterRegex and
SystemHeaders.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:84
+ /// and system files when --system-headers is used.
+ llvm::Optional<bool> SkipHeaders;
+
----------------
sammccall wrote:
> This is easy to confuse with e.g. HeaderFilterRegex and SystemHeaders, which
> control policy rather than just implementation strategy.
> I'd suggest calling this something like `PruneTraversal` which hints at the
> implementation strategy it controls but most importantly that it's *not*
> about whether you want warnings from the headers or not.
>
> (similarly for the flag)
Are you suggesting a change of flag name from --skip-headers to
--prune-traversal and internal option name from SkipHeaders to PruneTraversal?
So far, "traversal" seems to be an implementation term not yet used in
clang-tidy flags. But there is TraversalKind used in some clang-tidy checks.
They seem to mean different things from HeaderFilterRegex, and SystemHeaders.
The confusion exists between HeaderFilterRegex and SystemHeaders. Both should
really do skip-check-of-headers. Their "implementation" is like a short-cut to
only suppress output but not skipping the checks. When skip-headers is enabled
and maybe become the default, both HeaderFilterRegex and SystemHeaders will
make more sense.
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
