ArcsinX marked an inline comment as done. ArcsinX added a comment. In D93683#2469584 <https://reviews.llvm.org/D93683#2469584>, @sammccall wrote:
> In D93683#2469553 <https://reviews.llvm.org/D93683#2469553>, @ArcsinX wrote: > >> 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. > > Sorry, incomplete thought. I meant workspace/symbols would be broken for such > macros, but maybe that was acceptable (it's not a terrible common feature nor > symbol kind). Not great but we're trading one bug for another. > In particular, if we plan to fix both I don't think the *sequencing* matters. Ok. I will propose a separate patch for `fuzzyFind()` similar to this one. Thank you for review. ================ 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: > > 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`?) > > 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 > > Hmm, what if we made the return value of the indexedFiles functor a bitmask > instead of a single boolean (contains symbols, contains refs, ...)? This sounds reasonable. I will try to implement a solution for this problem based on your suggestion (in a separate patch) 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
