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

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 9 07:22:40 PST 2018

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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list