[PATCH] D94952: [clangd] Take into account what is in the index (symbols, references, etc.) at indexes merge

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for following up on this!



================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:264
 std::unique_ptr<SymbolIndex>
-FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle,
-                        size_t *Version) {
+FileSymbols::buildIndex(IndexType Type, IndexDataKind DataKind,
+                        DuplicateHandling DuplicateHandle, size_t *Version) {
----------------
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.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:433
     PreambleSymbols.update(
-        Uri, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
+        FilePath ? *FilePath : (consumeError(FilePath.takeError()), Uri),
+        std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
----------------
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?


================
Comment at: clang-tools-extra/clangd/index/Index.h:85
 
+enum class IndexDataKind : uint8_t {
+  None = 0,
----------------
This deserves a comment to motivate it... e.g.

Describes what data is covered by an index.
Indexes may contain symbols but not references from a file, etc.
This affects merging: if a staler index contains a reference but a
fresher one does not, we want to trust the fresher index *only*
if it actually includes references in general!


================
Comment at: clang-tools-extra/clangd/index/Index.h:85
 
+enum class IndexDataKind : uint8_t {
+  None = 0,
----------------
sammccall wrote:
> This deserves a comment to motivate it... e.g.
> 
> Describes what data is covered by an index.
> Indexes may contain symbols but not references from a file, etc.
> This affects merging: if a staler index contains a reference but a
> fresher one does not, we want to trust the fresher index *only*
> if it actually includes references in general!
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.


================
Comment at: clang-tools-extra/clangd/index/Index.h:98
+
+inline constexpr IndexDataKind operator|(IndexDataKind L, IndexDataKind R) {
+  return static_cast<IndexDataKind>(static_cast<uint8_t>(L) |
----------------
I'd also consider `explicit operator bool()` so you can write `if (Indexed(File) & IndexContents::References)`, but a question of taste.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94952/new/

https://reviews.llvm.org/D94952