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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/unittests/IndexActionTests.cpp:22
 using ::testing::ElementsAre;
+using testing::EndsWith;
 using ::testing::Not;
----------------
nit `using ::testing::EndsWith`

was this `add using` tweak ?


================
Comment at: clang-tools-extra/clangd/unittests/IndexActionTests.cpp:261
+  addFile(testPath("good.h"), R"cpp(
+    struct S { int s; };
+    void f1() { S s; }
----------------
maybe give the member a different name to distinguish it from the local var in function ?


================
Comment at: clang/include/clang/Index/IndexingOptions.h:40
+  // This prevents traversal, so skipping a struct means its declaration an
+  // members won't be indexed, but references that struct elsewhere will be.
+  // Currently this is only checked for top-level declarations.
----------------
s/references that/references to that


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80296