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

Reply via email to