[PATCH] D80296: [clangd] Don't traverse the AST within uninteresting files during indexing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang/lib/Index/IndexingAction.cpp:142
+    ShouldSkipFunctionBody =
+        [ShouldTraverseDecl(Opts.ShouldTraverseDecl)](const Decl *D) {
+          return !ShouldTraverseDecl(D);
----------------
sammccall wrote:
> kadircet wrote:
> > are we happy with the copy here ?
> > 
> > I am a little uneasy, as these functions can preserve state. Currently the one used in here and the one used in `IndexingContext::indexTopLevelDecl` are different copies.
> > 
> > Maybe rather progate a null function into the `IndexASTConsumer` and create this default lambda in there, making use of the function inside `IndexingContext::IndexOpts` without a copy?
> > are we happy with the copy here ?
> 
> Note that we already have a copy: we take IndexingOptions by const ref and createIndexingASTConsumer copies it, including the function. So anything that relies on the identity of the function passed in, or breaks semantics of copying, is busted already.
> 
> as for internal state (e.g. a cache)... I'd agree if this were a heavily used library function (find a way to capture by pointer/shared_ptr). But it's not, and neither of the in-tree callers work this way - I'm not sure it's worth spending API complexity on.
> 
> > Maybe rather progate...
> 
> Hmm, I like the idea that these are clearly separate options at the lower internal layer, and that blurs the lines a bit.
> (I'd like *more* making them completely the same, but halfway isn't a happy place to be I think)
> as for internal state (e.g. a cache)... I'd agree if this were a heavily used library function (find a way to capture by pointer/shared_ptr). But it's not, and neither of the in-tree callers work this way - I'm not sure it's worth spending API complexity on.

SG. I wasn't concerned about the copying but rather lambda invoked in `ShouldSkipFunctionBody` and `ShouldTraverseDecl` being different. I agree that preserving internal state is not that important for current in-tree uses, just wanted to make sure the discrepancy didn't go unnoticed :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80296