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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 5 inline comments as done.
sammccall added a comment.

In D80296#2053189 <https://reviews.llvm.org/D80296#2053189>, @adamcz wrote:

> Correct me if I'm wrong, but this might lead to a file never being updated in the index, even after edit, right?
>  Let's say you have file a.cpp that includes a.h. a.h has
>  namespace foo {
>
>   #define EXPAND(foo) whatever
>   #include "b.h"
>
> }
>  all nodes from b.h will be inside the namespace node in a.h. If a.h did not change, b.h will never be indexed.


Yes, there's some potential regression here. It exists if:

- the including file is unmodified, the included is modified
- the including file defines the top-level decl (e.g. enclosing namespaces), otherwise change has no effect
- the symbols are considered part of the included file for sharding purposes, otherwise this already didn't work. (I think they are in most cases)

I think I'm comfortable throwing this in the "clangd is designed for well-structured codebases" bucket, along with non-self-contained files in other contexts (b.h is necessarily not self-contained here). I'm also comfortable saying tablegen isn't a well-structured part of LLVM :-)
And for preamble index, (which doesn't yet have this optimization) I think that's the end of the story.
However, this being persistently sticky *forever* with the background index does seem pretty horrible. I can't see an elegant fix though, and a 33% speedup for background index is an important win (it's a top clangd complaint).

Any bright ideas? If not, maybe we land this and try to fix it somehow later, or just accept it as one of our speed/accuracy tradeoffs.



================
Comment at: clang-tools-extra/clangd/unittests/IndexActionTests.cpp:22
 using ::testing::ElementsAre;
+using testing::EndsWith;
 using ::testing::Not;
----------------
kadircet wrote:
> nit `using ::testing::EndsWith`
> 
> was this `add using` tweak ?
Yes, it was.
I feel a bit silly as I'm well aware of the behaviour here and have argued it's the right thing, and wrote `testing::EndsWith` inline, extracted it, and didn't check the output (as it was offscreen).

@adamcz FYI
If we see this a lot maybe we should consider something like "if all existing usings in the block are globally qualified...". Current behavior is definitely defensible though.


================
Comment at: clang/lib/Index/IndexingAction.cpp:142
+    ShouldSkipFunctionBody =
+        [ShouldTraverseDecl(Opts.ShouldTraverseDecl)](const Decl *D) {
+          return !ShouldTraverseDecl(D);
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80296