ArcsinX added inline comments.
================ Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:292 + for (const auto &Sym : *Slab) { + if (Sym.Definition) + Files.insert(Sym.Definition.FileURI); ---------------- kadircet wrote: > it feels weird to choose one or the other here, why not just insert both > (after checking if definition exists, ofc). > > We are likely to have a merged symbol information anyway, and the logic here > will likely result in no index owning the header files, unless there are some > symbols *defined* (not just declared) in it. > > This will likely result in some overshooting as knowing about a symbols > declaration location doesn't mean indexing that particular file. But so does > the current version, as knowing about definition location might be because of > a merged symbol, rather than indexing of particular file. I will try to explain on example: - **test.h** ``` void f(); ``` - **test.c** ``` #include "test.h" void f() { } ``` - compile_commands.json contains a command for **test.c** compilation. Scenario: - open **test.c** - try to find all references for `f()` For that scenario result for `find all references` will be incorrect if both (decl and def) files are in the file list because: - decl location is inside **test.h** - def location is inside **test.c** - the file list for the main file index contains **test.h** and **test.c** - the main file index does not contain references from **test.h** - the background (static) index contains references from **test.c**, but results from the background index will be skipped, because **test.h** is in the main file (dynamic) index file list. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97535/new/ https://reviews.llvm.org/D97535 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits