[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:217
 
+bool shouldIndexFile(const Decl& D, const SymbolCollector::Options &Opts,
+                     llvm::DenseMap<FileID, bool> *FilesToIndexCache) {
----------------
nit: this is very easily confused with the callback with the same name on the call sites.


================
Comment at: clangd/index/SymbolCollector.cpp:220
+  auto &SM = D.getASTContext().getSourceManager();
+  auto Loc = findNameLoc(&D);
+  return shouldIndexFile(SM, SM.getFileID(Loc), Opts, FilesToIndexCache);
----------------
This is following the implementation detail of how location is generated in `addDeclaration`/`addDefinition`. I think we could put the filtering into `addDeclaration`/`addDeclaration` to avoid the duplicate here (i.e. return nullptr if decl is filtered like what you did in the previous revision?).


================
Comment at: clangd/index/SymbolCollector.cpp:410
+    if (!BasicSymbol)
+      BasicSymbol = addDeclaration(
+          *cast<NamedDecl>(OriginalDecl.getCanonicalDecl()), *ID);
----------------
`OriginalDecl.getCanonicalDecl()` might not be the one we prefer. I think we should check all `redecls` and pick the preferred one? 


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:526
 
+TEST_F(SymbolCollectorTest, ShouldIndexFile) {
+  const std::string IgnoredHeader = R"(
----------------
It's not trivial to tell what are the cases we are testing here. Maybe add some comments?

There are a few cases that are interesting. For examples:
- Symbol is declared in a filtered header and defined in a index file.
- Symbol is declared in a indexed file and defined in a filter file.
- Symbol has forward declaration and canonical declaration in filtered header and definition in indexed file.
- ...

It might be clearer to split them into multiple cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54300