[PATCH] D54300: [clangd] Respect shouldIndexFile when collecting symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:386
   const Symbol *BasicSymbol = Symbols.find(*ID);
-  if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-    BasicSymbol = addDeclaration(*ND, std::move(*ID));
-  else if (isPreferredDeclaration(OriginalDecl, Roles))
-    // If OriginalDecl is preferred, replace the existing canonical
-    // declaration (e.g. a class forward declaration). There should be at most
-    // one duplicate as we expect to see only one preferred declaration per
-    // TU, because in practice they are definitions.
-    BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
-
-  if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
-    addDefinition(OriginalDecl, *BasicSymbol);
+  auto shouldIndexSymbol = [&](SourceLocation TargetLoc) {
+    return shouldIndexFile(SM, SM.getFileID(TargetLoc), Opts,
----------------
`shouldIndexLoc` seems more accurate?


================
Comment at: clangd/index/SymbolCollector.cpp:392
+  if (!BasicSymbol) {
+    auto DeclSLoc = findNameLoc(ND);
+    if (shouldIndexSymbol(DeclSLoc))
----------------
`DeclSLoc` is a bit confusing (SLoc is used somewhere else). Maybe `DeclSymLoc` or simply `DeclLoc`?


================
Comment at: clangd/index/SymbolCollector.cpp:397
+  }
+  if (isPreferredDeclaration(OriginalDecl, Roles)) {
+    auto DeclSLoc = findNameLoc(&OriginalDecl);
----------------
We would be doing redundant work if `OriginalDecl == ND`?


================
Comment at: clangd/index/SymbolCollector.cpp:409
+    auto DefLoc = findNameLoc(&OriginalDecl);
+    if (!shouldIndexSymbol(DefLoc) && !BasicSymbol)
+      return true;
----------------
Could you document here why we are also checking `!BasicSymbol ` here? 


================
Comment at: clangd/index/SymbolCollector.cpp:571
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
+                                              SourceLocation DeclSLoc,
                                               SymbolID ID) {
----------------
nit: `s/DeclSLoc/DeclLoc`? and maybe add documentation about why we need this?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:598
+                  Pair(findSymbol(Symbols, "Foo2").ID,
+                       RefsInFiles(Files({IndexedHeaderURI}))),
+                  Pair(findSymbol(Symbols, "Foo3").ID,
----------------
I think making `RefsInFile` take a single file and using it in combination with `anyOf`/`allOf` would be more straightforward.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54300