ArcsinX added a comment.
In D93683#2468842 <https://reviews.llvm.org/D93683#2468842>, @sammccall wrote:
> I'm not 100% sure this bug would block adding the equivalent change for
> `fuzzyFind` though - it'll affect documentSymbol but not code-completion IIUC
> (because of the workaround I mentioned above). And these symbols are pretty
> rare.
With the similar change for `fuzzyFind()`, `WorkspaceSymbols.Macros` test
fails. But maybe I can create one more review for this and we will discuss it
there.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:434
+ PreambleSymbols.update(
+ *FilePath, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
+ std::make_unique<RefSlab>(),
----------------
sammccall wrote:
> ArcsinX wrote:
> > We do not need this change to fix behaviour for removed definition, but
> > this is the only one place where we use URI instead of path as a key.
> Yeah... nevertheless the key is totally arbitrary here, so we might as well
> use whatever is most convenient/cheapest.
It's not arbitrary after D93393 (we use these keys to create the list of
indexed files).
But as it turns, the problem is deeper... I will try to explain.
Preamble index does not contain references, so with that change our logic about
"file X is in the dynamic index, so toss references with location in X from the
static index" breaks here, seems we can not think about preamble index that it
is a part of the dynamic index for the background index (static). I wonder why
tests do not catch this, but with this change find-all-references does not
contain refs from headers for which we have preamble index, that's why I
reverted this. Maybe we need to add some comment here about this behavior.
Example:
`test.h`
```
void test();
```
`test.c`
```
#include "test.h"
void te^st {}
```
- didOpen `test.c`
- references (where ^ is)
With this change only 1 element will be returned (definition of `test` in
`test.c` and nothing from `test.h`)
I am not sure what we can do here, but I think we need some special merge for
preamble and main file indexes (another merge class for this case instead of
`MergedIndex`?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93683/new/
https://reviews.llvm.org/D93683
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
