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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
