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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
