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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:310
+struct DeclFilter {
+  DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
+      : Context(Ctx), Sources(SM) {
----------------
chh wrote:
> sammccall wrote:
> > Ctx is ultimately unused, just take the regex instead?
> Context is used in skipFileID.
> 
Oops, I did miss this. Since we extract the header regex, can we also pull out the one boolean that we check later into a field? This would make it clearer how the context is used, since it's a large object.


================
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.
----------------
chh wrote:
> 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.
> 
OK, fair enough. However you seem to both be saying "let's do this for now" and "if traversal scope is unimportant for static-analyzer performance, we need never change this".
The main point of applying traversal scope uniformly is to avoid complexity and special cases, not to improve performance.

If we want to have this fairly unusual mechanism and require maintainers to understand it, then needs to be mitigated by a significant comment somewhere explaining:
 - what is being done (wrapping HandleTranslationUnitDecl in traversalscope load/restore logic for one consumer only)
 - how it's being done (keep an extra pointer to the "special" consumer so we can identify it)
 - why it's being done (ideally concrete problems in applying the scope everywhere, but at least why we don't expect this to be safe)

Coming up with a good answer to the "why" question is part of this change.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:99
     IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
     IO.mapOptional("UseColor", Options.UseColor);
   }
----------------
chh wrote:
> 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.
> 
> Why SystemHeaders is not mapped?

No idea, sorry.

> but SkipHeaders seems as important to be included here as HeaderFilterRegex and SystemHeaders.

I don't think so.

HeaderFilterRegex and SystemHeaders control policy, and it's reasonable for that to depend on the specific code (and therefore be dynamically configurable).
SkipHeaders only controls the mechanism/implementation strategy - why do we need to support different strategies for different files in the same clang-tidy invocation, rather than just rolling it out with a flag?

Moreover, the plan AIUI is for SkipHeaders to become the default and for the option to go away. We should try to avoid supporting it in config files if that's the case.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:84
+  /// and system files when --system-headers is used.
+  llvm::Optional<bool> SkipHeaders;
+
----------------
chh wrote:
> 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.
> 
> Are you suggesting a change of flag name from --skip-headers to --prune-traversal and internal option name from SkipHeaders to PruneTraversal?

Yes, sorry I left the comment at the wrong spot.

> So far, "traversal" seems to be an implementation term not yet used in clang-tidy flags.

That's right, because clang-tidy flags generally control behavior rather than implementation. But "skip headers" is different - it's intended to produce the same set of warnings, but faster by doing the filtering at a different step. In the long run I would prefer not to have this flag, but if we need it then it should have a name that emphasizes the implementation strategy, which is what it controls.

> But there is TraversalKind used in some clang-tidy checks. They seem to mean different things from HeaderFilterRegex, and SystemHeaders.

Yes, TraversalKind is unrelated (and IMO also poorly named).

> 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.

Sorry, I don't understand this paragraph. HeaderFilterRegex and SystemHeaders both help define which files are "in scope" for clang-tidy checks. This is the same whether --skip-headers=1 or --skip-headers=0, the only difference is at what stage the filtering is applied. I agree the --show-all-warnings flag should disable post-filtering for both.


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

https://reviews.llvm.org/D98710