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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This generally looks good now, mostly nits left.

The exception is the remaining complexity around stashing and restoring the traversal scope state.
If it's needed, it points to a deeper problem in static analyzer and we should understand it a bit (and whether it must be fixed as it will undo the performance characteristics of this feature in some circumstances).



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:310
+struct DeclFilter {
+  DeclFilter(ClangTidyContext &Ctx, SourceManager &SM)
+      : Context(Ctx), Sources(SM) {
----------------
Ctx is ultimately unused, just take the regex instead?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:346
 private:
+  bool handleTopLevelDecl(DeclGroupRef DG);
+  bool hasFilter() { return Filter.get() != nullptr; }
----------------
please find a different name for this function or merge the two together, having two different functions differing only by case is confusing


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:348
+  bool hasFilter() { return Filter.get() != nullptr; }
+  std::vector<Decl *> &getTraversalScope() { return TopLevelDecls; }
+
----------------
this function isn't needed - it's a trivial accessor called once in a place where we can access the field directly.
Similarly with hasFilter. The code accessing the fields directly reads just as clearly.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:428
 
+bool DeclFilter::skipFileID(SourceLocation Location, FileID FID) {
+  // do not skip the main file
----------------
can you move the impl of the members next to the class?


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


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:608
       std::move(Consumers), std::move(Profiling), std::move(Finder),
-      std::move(Checks));
+      FinderASTConsumerPtr, SharedDeclFilter, std::move(Checks));
 }
----------------
move SharedDeclFilter


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:473
+    MatchFinder::matchAST(Context);
+    Context.setTraversalScope(SavedScope);
+  } else {
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:99
     IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
     IO.mapOptional("UseColor", Options.UseColor);
   }
----------------
Maybe an explicit comment that ShowAllHeaders, SkipHeaders are runtime/optimization options that are configured at the command-line only, and therefore not mapped here


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:79
 
+  /// Show all warnings, including warnings from all header files.
+  llvm::Optional<bool> ShowAllWarnings;
----------------
This overrides (and is therefore grouped with) HeaderFilterRegex, SystemHeaders.
So the order should probably be
HeaderFilterRegex
SystemHeaders
ShowAllWarnings
SkipHeaders

Comment should comment should explicitly say both "This overrides HeaderFilterRegex, ..." and "This is an option intended for testing/debugging clang-tidy".


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:84
+  /// and system files when --system-headers is used.
+  llvm::Optional<bool> SkipHeaders;
+
----------------
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)


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:144
   MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
-  ~MatchFinder();
+  virtual ~MatchFinder();
+
----------------
chh wrote:
> 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.
> 
> Do you know the reason it cannot have subclass?

Because widely-used classes need to decide whether to accept the maintenance cost of supporting inheritance or not, and the author judged that the (conceptual) complexity of supporting it outweighs the things enabled by doing so.


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

https://reviews.llvm.org/D98710