[PATCH] D104071: [AST] Include the TranslationUnitDecl when traversing with TraversalScope

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D104071#2811827 <https://reviews.llvm.org/D104071#2811827>, @sammccall wrote:

> This includes both the traversal change and the clangd changes to adapt to it, since there ended up being few.
>
> I chickened out of the idea of changing the behavior of TranslationUnitDecl::decls(), for a couple of reasons:
>
> - having it apply to RecursiveASTVisitor only is similar to the previous version, so this is a slightly less invasive/risky change
> - it's actually slightly awkward to change the node's behavior as decls() is not virtual in DeclContext

Thanks, this seems a nice approach considering all trade-offs.

IIUC, the key point of the approach is that we change the RAV behavior on traversing TUDecl, to make it respect decls from traversal scope, which seems a reasonable mental model.
I think we should mention this in the patch description, and document somewhere around the RAV?

While it solves the critical problem in clang-tidy skip-headers, we should bear in mind that:

1. The default TU-decl behavior in RAV can be overrode in RAV's subclass (I bet it should be very rare, and we probably don't worry about this case )
2. users can still use "TU->decls()" to traverse all decls, which might cause clangd to deserialize preamble and get performance hit (this is an existing problem)

All in all, I'm up for this patch.



================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1500
+  auto Scope = D->getASTContext().getTraversalScope();
+  bool ScopeIsTrivial =
+      Scope.size() == 1 && isa<TranslationUnitDecl>(Scope.front());
----------------
I would use a more meaningful name -- it is unclear to me what "trivial" means here without reading the code very hard.

IIUC it distinguishes the conventional case ( a single translation-unit decl) vs the case (where e.g. the TraversalScope is set to main-file decls).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104071