Current indexes merge logic skip data from the static index if the file is in the dynamic index, but sometimes the dynamic index does not contain references (e.g. preamble (dynamic) index vs background (static) index).
This problem is masked with the fact, that the preamble index file list consists of file URI's and other indexes file lists consist of file paths.
This patch introduces the index contents (symbols, references, etc.), which makes indexes merge more flexible and makes it able to use URI's for the index file list.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for following up on this!
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
269 | I don't think it makes sense to specify this at index-build-time, should it be a constructor parameter instead? This value describes all the data previously passed to update(), so we must always update() with the same type of data. So this is really tied to the identify of the FileSymbols, rather than the buildIndex operation. | |
437 | Is this change related? It changes the key scheme for the preamble index from URIs to paths, but I'm not sure why. Do we have multiple URIs pointing to the same path? What are they concretely? | |
clang-tools-extra/clangd/index/Index.h | ||
85 | This deserves a comment to motivate it... e.g. Describes what data is covered by an index. | |
85 | This name is a bit vague for my taste (because both "data" and "kind" tend to get attached to everything, they lose their meaning) Maybe IndexContents? This is still vague but at least describes the relationship between the index and the e.g. Symbols. | |
98 | I'd also consider explicit operator bool() so you can write if (Indexed(File) & IndexContents::References), but a question of taste. |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 | This is the main thing in this patch. I will try to explain. |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 |
This is a bit incorrect. |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 | Oooh... I'm not sure how I misunderstood the original so much :-( And I missed it in this patch description as well, apologies. My impression was that the file list was derived from the index data, rather than from the keys, which were always intended to be opaque/arbitrary. It feels like using URIs extracted from symbols might not be *completely* robust. Because having CanonicalDeclaration etc set to a file might not line up exactly with the idea that we indexed the file. But we do use this partitioning for FileShardedIndex, so it has to work well enough. The advantage of using the extracted URIs would be: also works for non-file-sharded indexes like --index-file, avoid a bunch of conversion between URI and path, and we get to keep the simpler/flexible design for FileSymbols where the key is opaque. Does this seem feasible to you? |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 | I also do not like these path <=> URI conversions. But what about empty files?, e.g.:
Unsure, maybe it's ok to ignore the problem with empty files, seems this is the only case when the file was indexed, but we have no symbols located there. Overall, I like the idea to use URI's instead of paths. I think we could implement it first as a separate patch and after that return to this one. What do you think? |
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 | Right, completely empty files are going to be mishandled., in the same way that before your changes we mishandled files that had no results from the query. I do still think this is the way to go though, because:
That sounds great if you don't mind! |
- Set IdxContents at FileSymbols object breation instead of at FileSymbols::buildIndex() call.
- Revert change of the preamble index key scheme
- Add comment for IndexContents
- IndexDataKind => IndexContents
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 |
Seems we have no reason =) | |
clang-tools-extra/clangd/index/Index.h | ||
98 | Unsure how I can do this =(
|
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
437 |
Seems we need to land this patch first. Without index contents it's impossible to use URI's everywhere in the index file list (because the preamble index does not contain references, but without this patch we do not have this information at indexes merge). |
Sorry, forgot to stamp this!
clang-tools-extra/clangd/index/FileIndex.cpp | ||
---|---|---|
417 | seems we include relations too? | |
420 | (this should take into account CollectMainFileRefs, but actually if you sync to HEAD it's always true now) | |
clang-tools-extra/clangd/index/FileIndex.h | ||
74 | It seems error-prone to have a default here. If we must have a default, All seems to make more sense than None. |
- Remove default value of IdxContents in FileSymbols constructor.
- Fix index contents for the preamble index (Symbols => Symbols|Relations).
- Rebase
It seems error-prone to have a default here.
(I guess it's for making tests shorter? Are there enough callsites for it to matter?)
If we must have a default, All seems to make more sense than None.