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