[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/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