sammccall added inline comments.
================
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)),
----------------
ArcsinX wrote:
> sammccall wrote:
> > 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?
> This is the main thing in this patch. I will try to explain.
> We use these keys to create the file list, which is used by `indexedFiles()`.
> Currently, the preamble index contains URI's instead of paths (as a file
> list), that leads to the function returned by `PreambleIndex::indexedFiles()`
> always return `false` (because we pass to this function paths, not URI's).
> So, we always take data from the preamble index (but maybe we should not in
> some cases).
>
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.
(At various times, these have been filenames, URIs, and other things IIRC. And
until relatively recently, the preamble index keys were the file the preamble
was built from, not the file containing the symbol!)
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?
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
