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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:217
 
+bool shouldIndexFile(const Decl& D, const SymbolCollector::Options &Opts,
+                     llvm::DenseMap<FileID, bool> *FilesToIndexCache) {
----------------
ioeric wrote:
> nit: this is very easily confused with the callback with the same name on the call sites.
removed it.


================
Comment at: clangd/index/SymbolCollector.cpp:220
+  auto &SM = D.getASTContext().getSourceManager();
+  auto Loc = findNameLoc(&D);
+  return shouldIndexFile(SM, SM.getFileID(Loc), Opts, FilesToIndexCache);
----------------
ioeric wrote:
> 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?).
as discussed, we decide to move the filter logic out of `addDeclaration`/`addDefinition`, and also move out the `findNameLoc` logic.


================
Comment at: clangd/index/SymbolCollector.cpp:410
+    if (!BasicSymbol)
+      BasicSymbol = addDeclaration(
+          *cast<NamedDecl>(OriginalDecl.getCanonicalDecl()), *ID);
----------------
ioeric wrote:
> `OriginalDecl.getCanonicalDecl()` might not be the one we prefer. I think we should check all `redecls` and pick the preferred one? 
As discussed, `canonicalDecl` is always the decl we preferred here. If the definition `decl` is preferred, it will be handled in above `isPreferredDeclaration` code.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:526
 
+TEST_F(SymbolCollectorTest, ShouldIndexFile) {
+  const std::string IgnoredHeader = R"(
----------------
ioeric wrote:
> 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.
Added comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54300