sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
In D93683#2467631 <https://reviews.llvm.org/D93683#2467631>, @ArcsinX wrote: > I also want to propose the similar patch for fuzzyFind() (as a part of this > patch or a separate one), but I faced the following problem: > > `Test.cpp` > > #define FOO 1 > > This example creates empty dynamic index for main file symbols and static > index for preamble symbols (`FOO`). The location of `FOO` is inside > `Test.cpp`, so (according to the logic that the static index could be stale) > we should toss `FOO` from the static index (i.e. preamble symbols), but this > behaviour is incorrect... Any advice? Ugh, this case manages to fall through the cracks of our model every time. Originally the idea was: - features traverse the AST for the current file, and consult the index for headers - uh... macros do something analogous to that - this lines up with the preamble/parsedAST split But macros defined in the preamble section aren't in headers but also aren't parsed in parsedAST, so they're really awkward to handle. We end up playing whack-a-mole with these bugs. (See loadMainFilePreambleMacros in CodeComplete.cpp). In this case, it seems reasonable to include the macros in the dynamic index for the file. This is a bit of work (requires handling MainFileMacros::Name in SymbolCollector::handleMacros I guess) but is probably less work than excluding the symbols from the index entirely and having to work around this in every feature. It *also* seems reasonable to exclude these main-file symbols from the preamble index. For one thing, the preamble index will include all the files with any symbol defined in `indexedFiles()`, and the whole file isn't indexed! 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. ================ 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: > 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. ================ Comment at: clang-tools-extra/clangd/index/Merge.cpp:81 Static->lookup(Req, [&](const Symbol &S) { + if (DynamicContainsFile(S.Definition ? S.Definition.FileURI + : S.CanonicalDeclaration.FileURI)) ---------------- this seems OK because we expect the definition to see the canonical declaration - subtle enough to be worth a comment. (The dumb version that always checks CanonicalDeclaration, and sometimes also checks Definition, would be a bit more obvious, but saving the lookup is probably worthwhile here) 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
