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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
