[PATCH] D93683: [clangd] Do not take stale definition from the static index.
Aleksandr Platonov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 22 23:27:22 PST 2020
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
More information about the cfe-commits
mailing list