sammccall added a comment. 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. ================ 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>(), ---------------- 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, ...)? 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
